asterixdb-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jianfeng Jia (Code Review)" <do-not-re...@asterixdb.incubator.apache.org>
Subject Change in asterixdb[master]: Avoid always merging old components in prefix policy
Date Fri, 09 Jun 2017 16:13:39 GMT
Jianfeng Jia has posted comments on this change.

Change subject: Avoid always merging old components in prefix policy
......................................................................


Patch Set 4:

(7 comments)

some minor comments.

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

Line 49:     private final static double MAX_MERGABLE_COMPONENT_SIZE_RATIO = 1.2;
maybe it's good to also put the source of this magic number?


Line 143:         Collections.reverse(immutableComponents);
could you explain the reason of the reverse?


Line 256:     private Pair<Integer, Integer> getMergableComponentsIndex(List<ILSMDiskComponent>
immutableComponents) {
I think it's better to move this big chunk of comment before the function as Java doc?


Line 266:                     || immutableComponents.get(i).getState() != ComponentState.READABLE_UNWRITABLE)
{
just curious, to make sure the state check is consistent, is this function (or its caller)
synchronized or there is only one merge thread who also takes care of updating the state?


Line 276:                     // normally this shouldn't happen, but just make sure we skip
unmergeable components
if it's a suspicious state, it's better to add a warning log.


Line 284:                 boolean isLastComponent = j + 1 == immutableComponents.size() ?
true : false;
no need to use ternary?


Line 289:                     return Pair.of(i, j);
it feels like we should treat two part of the && differently for the else case? 

if `!canMerge` [i,j], we just increase j, 
if `canMerge` [i,j], 
    if `shouldMerge[i,j]`, return
    else increase i  and break

correct me if I'm wrong :-)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I464da3fed38cded0aee7b319a35664eae069a2ba
Gerrit-PatchSet: 4
Gerrit-Project: asterixdb
Gerrit-Branch: master
Gerrit-Owner: Luo Chen <cluo8@uci.edu>
Gerrit-Reviewer: Ian Maxon <imaxon@apache.org>
Gerrit-Reviewer: Jenkins <jenkins@fulliautomatix.ics.uci.edu>
Gerrit-Reviewer: Jianfeng Jia <jianfeng.jia@gmail.com>
Gerrit-Reviewer: Yingyi Bu <buyingyi@gmail.com>
Gerrit-Reviewer: abdullah alamoudi <bamousaa@gmail.com>
Gerrit-HasComments: Yes

Mime
View raw message