Murtadha Hubail has posted comments on this change. Change subject: [ASTERIXDB-2453] Add Improved Constant Merge Policy ......................................................................
Patch Set 2: (4 comments) https://asterix-gerrit.ics.uci.edu/#/c/2971/2/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/impls/AbstractLSMIndex.java File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/impls/AbstractLSMIndex.java: PS2, Line 905: public long getNumFlushes() throws HyracksDataException { : return ((AbstractLSMIndexFileManager) fileManager).getCurrentComponentSequence() + 1; : } You cannot use this value to get the correct number of flushes in the merge policy. If two or more flushes are secluded, then you will get a number that is higher than what you actually expect. In additional, this value could go backwards when component deletes is performed. Not sure if that's something that effects the algorithm but wanted to make sure that you are aware of it. If you would like to get the sequence number of the most recent disk component, you could try something like this: public Optional<Long> getLastestDiskComponentSequence() { if (diskComponents.isEmpty()) { return Optional.empty(); } final ILSMDiskComponent latestDiskComponent = diskComponents.get(0); final Set<String> lsmComponentPhysicalFiles = latestDiskComponent.getLSMComponentPhysicalFiles(); final String fileName = lsmComponentPhysicalFiles.stream().findAny() .orElseThrow(() -> new IllegalStateException("Component without any physical files")); return Optional.of(IndexComponentFileReference.of(fileName).getSequenceEnd()); } Please note that the merge policy is called even if the generated component is an EMPTY_COMPONENT (i.e. a result of components deletion). So, you might want to make sure the new algorithm wont be doing anything unexpected due to that. https://asterix-gerrit.ics.uci.edu/#/c/2971/2/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/impls/ConstantMergePolicy.java File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/impls/ConstantMergePolicy.java: Line 38: private static final Logger LOGGER = Logger.getLogger(ConstantMergePolicy.class.getName()); > MAJOR SonarQube violation: +1. Also, if you plan to use it, please use log4j2 loggers. Line 64: while ((treeDepth(depth) < numFlushes)) { > MAJOR SonarQube violation: +1 PS2, Line 144: boolean isMergeOngoing = isMergeOngoing(immutableComponents); : if (isMergeOngoing) { : return true; : } : return false; return isMergeOngoing(immutableComponents); -- To view, visit https://asterix-gerrit.ics.uci.edu/2971 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie5f83a4d5fdd3f036b823c906df1760f5110ae0a Gerrit-PatchSet: 2 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Luo Chen <[email protected]> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Michael Blow <[email protected]> Gerrit-Reviewer: Murtadha Hubail <[email protected]> Gerrit-Reviewer: Steven Jacobs <[email protected]> Gerrit-HasComments: Yes
