Not use ConcurrentHashMap in CoordinatorRuleManager.rules#9302
Not use ConcurrentHashMap in CoordinatorRuleManager.rules#9302jihoonson merged 1 commit intoapache:masterfrom
Conversation
There was a problem hiding this comment.
Please use Collections.emptyMap().
There was a problem hiding this comment.
Please use Collections.emptyMap().
There was a problem hiding this comment.
new HashMap() looks unnecessary.
There was a problem hiding this comment.
do you mean set rules directly, no need create newRules?
zhenxiao
changed the title
There was a problem hiding this comment.
I think this should be an AtomicReference of an ImmutableMap otherwise we can not guarantee that no one updates the map after it is set here.
There was a problem hiding this comment.
Good point. The value type (List<Rule>) should be immutable as well. I'm ok with using UnmodifiableMap and UnmodifiableList.
There was a problem hiding this comment.
+1 - I missed that.
Perhaps the best approach here is to keep the definition of the variable to AtomicReference<Map<String, List<Rule>>>
And add 2 unit tests to ensure
1 - we can not add to/ remove from the map
2 - we can not mutate the rules for an entry in a map.
This way if someone changes this behavior in the future we can catch it easily, and changing the type of Map/ List is easy as well.
There was a problem hiding this comment.
Oh yeah, I didn't mean the value type of rules should be UnmodifiableList<Rule>.
@suneet-s do you regard your comment as a blocker for this PR? Those tests would be definitely nice to have, but since any tests haven't rewritten for this class, I'm fine with adding all necessary tests including the ones you mentioned in a follow-up PR at once. @zhenxiao you can do in this PR if you want though.
There was a problem hiding this comment.
@jihoonson I think we need some way to verify that this change works and will not break in the future.
I understand it's not any worse than before, but I don't think we'll ever come back to add tests for this until we hit a bug - at which point it will be really hard to debug. It should be relatively quick to add those 2 tests so I think it's worth it. If there's a fast follow, that should be ok.
I'll leave it to you for final decision on whether this should be a blocker or not (I'm not sure what the apache rules are), but I think debugging this if someone changes the code to mutate the map would be very tough - I'd strongly recommend we add tests here to prevent that pain in the future.
There was a problem hiding this comment.
@zhenxiao would you consider @suneet-s's comment above? I think it would be nice to have. If you want to add them in this PR, I think you may need to create a new class, CoordinatorRuleManagerTest, and expose the rules via a method annotated with @VisibleForTesting to verify that the map inside the AtomicRefence is immutable. Another option could be creating an issue for adding those tests and doing in a follow-up PR.
There was a problem hiding this comment.
@jihoonson I'm not so familiar with this area of the code, so I'm happy to defer to your judgement. I like the tests you've described and think it's ok if tests come as a fast follow
@zhenxiao can you describe what manual testing you've done to verify this change? I'm sorry if this is coming across as nagging, I've just experienced a lot of pain trying to triage concurrency issues in the past.
There was a problem hiding this comment.
@suneet-s what kind of manual testings you think need to be done? The change in this PR looks pretty straightforward and I don't think this PR would lead to any sort of concurrency issues at least for now.
There was a problem hiding this comment.
thanks for your comments @suneet-s @jihoonson
updated to unmodifiableMap
I will file an issue to add tests after this PR is merged
|
@zhenxiao would you please update the title to be more accurate? Perhaps "Use HashMap instead of ConcurrentHashMap in CoordinatorRuleManager". |
zhenxiao
changed the title
4 participants
Fixes #9292
@leventov