Skip to content

KYLIN-4397 Use newLinkedHashMap for deterministic order in AssignmentUtil.java #1133

Merged
shaofengshi merged 1 commit intoapache:masterfrom
cpugputpu:use-newLinkedHashMap-in-AssignmentUtil.java
Mar 26, 2020
Merged

KYLIN-4397 Use newLinkedHashMap for deterministic order in AssignmentUtil.java #1133
shaofengshi merged 1 commit intoapache:masterfrom
cpugputpu:use-newLinkedHashMap-in-AssignmentUtil.java

Conversation

@cpugputpu
Copy link

This PR aims to solve the issue here: https://issues.apache.org/jira/browse/KYLIN-4397

The fix is to use newLinkedHashMap instead of newHashMap when initializing a Map. In this way, the non-deterministic behaviour is eliminated and the test will not suffer from the failure again. The code will be more stable.

@shaofengshi shaofengshi requested a review from hit-lacus March 3, 2020 06:52
@hit-lacus
Copy link
Member

hit-lacus commented Mar 4, 2020

I am a little confused at the moment.

From your message in JIRA issue, you found that error :

java.lang.AssertionError: expected:<2> but was:<1>
at org.apache.kylin.stream.coordinator.assign.DefaultAssignerTest.reBalanceTest(DefaultAssignerTest.java:165) when executing assertEquals(2, rsAssignment.size());

You claim that LinkedHashMap has guarantees to its order, and I agreed with it.
So you thought we should change the implementation of the result from HashMap to LinkedHashMap, right? Andresult is the return value of DefaultAssigner#reBalancePlan.

    public Map<Integer, Map<String, List<Partition>>> reBalancePlan(List<ReplicaSet> replicaSets,
            List<StreamingCubeInfo> cubes, List<CubeAssignment> existingAssignments) {
        Map<Integer, Map<String, List<Partition>>> newPlan = Maps.newHashMap();
        if (replicaSets == null || cubes == null || cubes.size() == 0 || replicaSets.size() == 0) {
            return newPlan;
    }

So, maybe you change the implementation of newPlan, right? Not in the AssignmentUtil.java.

Finally , I didn't reproduce this kind of error in my dev env. I felt confused too.

@hit-lacus
Copy link
Member

@cpugputpu Hi, if I misunderstand anything, please let me know, thank you in advance.

Copy link
Member

@hit-lacus hit-lacus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it is not necessary code change. Hope author will give more explanation.

Copy link
Contributor

@shaofengshi shaofengshi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 LGTM

@shaofengshi shaofengshi merged commit f768c71 into apache:master Mar 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants