Murtadha Hubail has posted comments on this change. Change subject: [ASTERIXDB-2204][STO] Fix implementations and usages of IIndexCursor ......................................................................
Patch Set 10: (12 comments) Tow general comments: All of these cursors are assumed to be not thread safe right? Also, I would revert all the changes on LogManager since they aren't related to this change. 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 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 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. PS10, Line 467: LOGGER.log(Level.INFO, "Getting log file ids from " + logDir); remove 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 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 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? 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 remove these three duplicates. 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? 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? 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 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 abstract methods? -- 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
