asterixdb-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Yingyi Bu (Code Review)" <do-not-re...@asterixdb.incubator.apache.org>
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:

(7 comments)

https://asterix-gerrit.ics.uci.edu/#/c/1788/4//COMMIT_MSG
Commit Message:

PS4, Line 8: 
Add more description into the commit message?   E.g., the usage of a shadow file.


https://asterix-gerrit.ics.uci.edu/#/c/1788/4/hyracks-fullstack/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/api/IPageManager.java
File hyracks-fullstack/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/api/IPageManager.java:

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?

    @Override
    public void clear() throws HyracksDataException {
        // initialize meta data page
        ITreeIndexMetadataFrame metaFrame = createMetadataFrame();
        ICachedPage metaNode = bufferCache.pin(BufferedFileHandle.getDiskPageId(fileId, getMetadataPageId()),
true);
        metaNode.acquireWriteLatch();
        try {
            metaFrame.setPage(metaNode);
            metaFrame.init();
            metaFrame.setRootPageId(1);
            metaFrame.setMaxPage(1);
        } finally {
            metaNode.releaseWriteLatch(true);
            bufferCache.flushDirtyPage(metaNode);
            bufferCache.unpin(metaNode);
        }
    }

IMO, we'd better make disk buffer cache really "READ_ONLY", so that we can get rid of latches
and dirty page handling later.


https://asterix-gerrit.ics.uci.edu/#/c/1788/4/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/api/ILSMDiskComponent.java
File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/api/ILSMDiskComponent.java:

PS4, Line 69: boolean exists();
It looks nobody calls that method, thus we probably shouldn't expose that.


https://asterix-gerrit.ics.uci.edu/#/c/1788/4/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/impls/AbstractLSMDiskComponent.java
File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/impls/AbstractLSMDiskComponent.java:

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?


https://asterix-gerrit.ics.uci.edu/#/c/1788/4/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/impls/LSMHarness.java
File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/impls/LSMHarness.java:

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 https://asterix-gerrit.ics.uci.edu/1788
To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5fe10965734b60776100580c9568a0f954d6cb58
Gerrit-PatchSet: 4
Gerrit-Project: asterixdb
Gerrit-Branch: master
Gerrit-Owner: abdullah alamoudi <bamousaa@gmail.com>
Gerrit-Reviewer: Jenkins <jenkins@fulliautomatix.ics.uci.edu>
Gerrit-Reviewer: Luo Chen <cluo8@uci.edu>
Gerrit-Reviewer: Murtadha Hubail <hubailmor@gmail.com>
Gerrit-Reviewer: Till Westmann <tillw@apache.org>
Gerrit-Reviewer: Yingyi Bu <buyingyi@gmail.com>
Gerrit-Reviewer: abdullah alamoudi <bamousaa@gmail.com>
Gerrit-HasComments: Yes

Mime
View raw message