abdullah alamoudi has posted comments on this change. Change subject: [ASTERIXDB-2204][STO] Fix implementations and usages of IIndexCursor ......................................................................
Patch Set 10: (12 comments) https://asterix-gerrit.ics.uci.edu/#/c/2324/10/asterixdb/asterix-app/src/main/resources/checkpointing.conf File asterixdb/asterix-app/src/main/resources/checkpointing.conf: Line 1: ; Licensed to the Apache Software Foundation (ASF) under one > this isn't needed anymore, the test was fixed in another patch Done https://asterix-gerrit.ics.uci.edu/#/c/2324/10/asterixdb/asterix-app/src/test/resources/log4j2-test.xml File asterixdb/asterix-app/src/test/resources/log4j2-test.xml: Line 51 > revert Done https://asterix-gerrit.ics.uci.edu/#/c/2324/10/asterixdb/asterix-transactions/src/main/java/org/apache/asterix/transaction/management/service/logging/LogManager.java File asterixdb/asterix-transactions/src/main/java/org/apache/asterix/transaction/management/service/logging/LogManager.java: PS10, Line 402: logFileIds.remove(logFileIds.size() - 1); > this is already done inside the for loop. I am not sure I trust the code below to do the right thing. I've seen before that it was in some corner cases deleting the log file being written to. This is a simple guard against such bugs and it doesn't hurt. PS10, Line 467: LOGGER.log(Level.INFO, "Getting log file ids from " + logDir); > remove Done PS10, Line 475: if (!fileLogDir.isDirectory()) { : LOGGER.log(Level.INFO, "log dir " + logDir + " exists but it is not a directory. returning empty list"); : return Collections.emptyList(); : } > remove this. It should be illegal state if kept Done PS10, Line 488: if (logFileNames == null) { : LOGGER.log(Level.INFO, "listing of log dir (" + logDir + ") files returned null. " : + "Either an IO error occurred or the dir was just deleted by another thread"); : return Collections.emptyList(); : } > remove Why? I think that if we hit such a case, we should be able to tell from the logs. Otherwise, you will hit a NullPointerException. https://asterix-gerrit.ics.uci.edu/#/c/2324/10/hyracks-fullstack/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/api/IIndexOperationContext.java File hyracks-fullstack/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/api/IIndexOperationContext.java: PS10, Line 47: All other calls after this method is invoked must throw exceptions > this isn't the case right now, right? Correct and we don't have any tests for this https://asterix-gerrit.ics.uci.edu/#/c/2324/10/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/util/DestroyUtils.java File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/util/DestroyUtils.java: PS10, Line 30: IIndexOperationContext > I would add a functional interface IDestroyable and have them extend it and Done. IDestroyable is effectively java.io.Closeable. The problem is that cursors already have open/close pairs. Maybe the right way is to extend Closeable and rename open/close in cursors to begin/end https://asterix-gerrit.ics.uci.edu/#/c/2324/10/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-invertedindex/src/main/java/org/apache/hyracks/storage/am/lsm/invertedindex/impls/LSMInvertedIndex.java File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-invertedindex/src/main/java/org/apache/hyracks/storage/am/lsm/invertedindex/impls/LSMInvertedIndex.java: PS10, Line 400: cursor > when is this destroyed? Done https://asterix-gerrit.ics.uci.edu/#/c/2324/10/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-invertedindex/src/main/java/org/apache/hyracks/storage/am/lsm/invertedindex/impls/LSMInvertedIndexRangeSearchCursor.java File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-invertedindex/src/main/java/org/apache/hyracks/storage/am/lsm/invertedindex/impls/LSMInvertedIndexRangeSearchCursor.java: PS10, Line 105: try { > should this be after search? Done https://asterix-gerrit.ics.uci.edu/#/c/2324/10/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-invertedindex/src/main/java/org/apache/hyracks/storage/am/lsm/invertedindex/ondisk/FixedSizeElementInvertedListCursor.java File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-invertedindex/src/main/java/org/apache/hyracks/storage/am/lsm/invertedindex/ondisk/FixedSizeElementInvertedListCursor.java: PS10, Line 73: if (currentElementIx < numElements) { : return true; : } else { : return false; : } > return currentElementIx < numElements Done https://asterix-gerrit.ics.uci.edu/#/c/2324/10/hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/EnforcedIndexCursor.java File hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/EnforcedIndexCursor.java: PS10, Line 30: class > shouldn't this be an abstract class with the // do nothing methods as abstr We can make it... I just wanted to have a test on this class by itself to make sure the enforcement is correct. -- To view, visit https://asterix-gerrit.ics.uci.edu/2324 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: I98a7a8b931eb24dbe11bf2bdc61b754ca28ebdf9 Gerrit-PatchSet: 10 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: abdullah alamoudi <bamou...@gmail.com> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Jenkins <jenk...@fulliautomatix.ics.uci.edu> Gerrit-Reviewer: Luo Chen <cl...@uci.edu> Gerrit-Reviewer: Michael Blow <mb...@apache.org> Gerrit-Reviewer: Murtadha Hubail <mhub...@apache.org> Gerrit-Reviewer: Taewoo Kim <taew...@uci.edu> Gerrit-Reviewer: Taewoo Kim <wangs...@gmail.com> Gerrit-Reviewer: Till Westmann <ti...@apache.org> Gerrit-Reviewer: abdullah alamoudi <bamou...@gmail.com> Gerrit-HasComments: Yes