asterixdb-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "abdullah alamoudi (Code Review)" <do-not-re...@asterixdb.incubator.apache.org>
Subject Change in asterixdb[master]: Cleanup Buffer Cache
Date Fri, 16 Jun 2017 15:59:30 GMT
abdullah alamoudi has posted comments on this change.

Change subject: Cleanup Buffer Cache
......................................................................


Patch Set 7:

(6 comments)

https://asterix-gerrit.ics.uci.edu/#/c/1840/6//COMMIT_MSG
Commit Message:

PS6, Line 16: 7. open non existing file is not allowed.
> I think this should be allowed if WRITE is part of the mode.  Otherwise, I 
if it doesn't exist, one should call the create method first...
The more defined the behavior is, the more robust the storage can get!


https://asterix-gerrit.ics.uci.edu/#/c/1840/6/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/context/FileMapManager.java
File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/context/FileMapManager.java:

PS6, Line 47: fileId
> I wonder whether all methods have to be synchronized?  It doesn't make sens
This doesn't need to be synchroinzed. This is accessed by buffer cache which synchronize on
fileInfoMap. complicating this further seems wasteful to me...

Even if we decide to change this, it should be outside the scope of this change.


https://asterix-gerrit.ics.uci.edu/#/c/1840/6/hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/util/IoUtil.java
File hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/util/IoUtil.java:

PS6, Line 30: /**
> A brief doc for the class and each public method?
Done


https://asterix-gerrit.ics.uci.edu/#/c/1840/6/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-nc/src/main/java/org/apache/hyracks/control/nc/io/FileHandle.java
File hyracks-fullstack/hyracks/hyracks-control/hyracks-control-nc/src/main/java/org/apache/hyracks/control/nc/io/FileHandle.java:

PS6, Line 55: READ_WRITE:
            :                 if (!fileRef.getFile().exists()) {
            :                     throw HyracksDataException.create(ErrorCode.FILE_DOES_NOT_EXIST,
fileRef.getAbsolutePath());
            :                 }
> Why only check file existence in READ_WRITE?
the only check for read write is because read will fail automatically while read write will
create the file.

I think auto creation is bad and dangerous. If you want to create a file, create it and then
open it.

if open does open and create if not found, then you risk not knowing about problems and code
bugs!

For example, if you expect a file to be there and it is not there, this will let you know.
while if it simply creates the file, then you might not know.


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

PS6, Line 81:    /**
            :      * @return true if the index dir exists, false otherwise
            :      */
            :     public boolean exists();
> I don't think it is necessary if we auto-create dir if not exits.
Auto creating the dir is bad!

and this is needed because we want to check if someone calls index.create. If the index directory
exists, then this means that the index exists and the new create should fail.


https://asterix-gerrit.ics.uci.edu/#/c/1840/6/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-invertedindex/src/main/java/org/apache/hyracks/storage/am/lsm/invertedindex/api/IInvertedIndex.java
File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-invertedindex/src/main/java/org/apache/hyracks/storage/am/lsm/invertedindex/api/IInvertedIndex.java:

PS6, Line 43:     /**
            :      * Purge the index files out of the buffer cache.
            :      * Can only be called if the caller is absolutely sure the files don't contain
dirty pages
            :      *
            :      * @throws HyracksDataException
            :      *             if the index is active
            :      */
            :     void purge() throws HyracksDataException;
> Maybe I missed sth. Why can destory() take care of that internally?
purge() has nothing to do with destry(). it is a quick way to tell the buffer cache to remove
the file pointer since it is being closed and all of its pages are not dirty.


-- 
To view, visit https://asterix-gerrit.ics.uci.edu/1840
To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I15565b07afdc94ac74c608bfe4480fa09dcf8f1c
Gerrit-PatchSet: 7
Gerrit-Project: asterixdb
Gerrit-Branch: master
Gerrit-Owner: abdullah alamoudi <bamousaa@gmail.com>
Gerrit-Reviewer: Ian Maxon <imaxon@apache.org>
Gerrit-Reviewer: Jenkins <jenkins@fulliautomatix.ics.uci.edu>
Gerrit-Reviewer: Michael Blow <mblow@apache.org>
Gerrit-Reviewer: Murtadha Hubail <hubailmor@gmail.com>
Gerrit-Reviewer: Yingyi Bu <buyingyi@gmail.com>
Gerrit-Reviewer: abdullah alamoudi <bamousaa@gmail.com>
Gerrit-HasComments: Yes

Mime
View raw message