asterixdb-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Yingyi Bu (Code Review)" <>
Subject Change in asterixdb[master]: Enable Component rollback in LSM Indexes
Date Fri, 02 Jun 2017 22:01:45 GMT
Yingyi Bu has posted comments on this change.

Change subject: Enable Component rollback in LSM Indexes

Patch Set 4:

Commit Message:

PS4, Line 8: 
Add more description into the commit message?   E.g., the usage of a shadow file.
File hyracks-fullstack/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/api/

PS4, Line 171: clear the page manager (cancel flush or merge)
I couldn't understand how the implementation matches the description?  (It's likely that I
missed sth.)  

The code seems to reset the index?   If so, why is this mandatory rather than just calling
newComponent.destroy() or let newComponent.destroy() takes care of ongoing flush operation?

    public void clear() throws HyracksDataException {
        // initialize meta data page
        ITreeIndexMetadataFrame metaFrame = createMetadataFrame();
        ICachedPage metaNode =, getMetadataPageId()),
        try {
        } finally {

IMO, we'd better make disk buffer cache really "READ_ONLY", so that we can get rid of latches
and dirty page handling later.
File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/api/

PS4, Line 69: boolean exists();
It looks nobody calls that method, thus we probably shouldn't expose that.
File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/impls/

PS4, Line 79: fileRef.getRelativePath().length() - 1
This is tightly coupled with the current suffixes which are all one character.  Can you make
it as a general extension name replacement?
File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/impls/

PS4, Line 210: {
I wonder whether there is a better approach?

Why do we do it in exitComponent?  Especially in the synchronized block of opTracker? Removing
a file from the underlying OS can take a while.

PS4, Line 218: AbstractLSMDiskComponent
cast is not needed.

PS4, Line 223: markAsValid
markAsValid will result in a disk force, i.e., an I/O operation.  It doesn't look proper to
do it in a synchronized block (of op tracker) which block other partitions, DMLs, etc.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I5fe10965734b60776100580c9568a0f954d6cb58
Gerrit-PatchSet: 4
Gerrit-Project: asterixdb
Gerrit-Branch: master
Gerrit-Owner: abdullah alamoudi <>
Gerrit-Reviewer: Jenkins <>
Gerrit-Reviewer: Luo Chen <>
Gerrit-Reviewer: Murtadha Hubail <>
Gerrit-Reviewer: Till Westmann <>
Gerrit-Reviewer: Yingyi Bu <>
Gerrit-Reviewer: abdullah alamoudi <>
Gerrit-HasComments: Yes

View raw message