You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2020/02/02 06:16:14 UTC
[GitHub] [druid] zhenxiao opened a new pull request #9302: Use HashMap for
CoordinatorRuleManager.rules
zhenxiao opened a new pull request #9302: Use HashMap for CoordinatorRuleManager.rules
URL: https://github.com/apache/druid/pull/9302
Fixes #9292
@leventov
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a change in pull request #9302: Not
use for CoordinatorRuleManager.rules
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #9302: Not use for CoordinatorRuleManager.rules
URL: https://github.com/apache/druid/pull/9302#discussion_r373878635
##########
File path: server/src/main/java/org/apache/druid/server/router/CoordinatorRuleManager.java
##########
@@ -58,7 +57,7 @@
private final ObjectMapper jsonMapper;
private final Supplier<TieredBrokerConfig> config;
- private final AtomicReference<ConcurrentHashMap<String, List<Rule>>> rules;
+ private final AtomicReference<Map<String, List<Rule>>> rules;
Review 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #9302: Use
HashMap for CoordinatorRuleManager.rules
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #9302: Use HashMap for CoordinatorRuleManager.rules
URL: https://github.com/apache/druid/pull/9302#discussion_r373868013
##########
File path: server/src/main/java/org/apache/druid/server/router/CoordinatorRuleManager.java
##########
@@ -149,7 +148,7 @@ public void poll()
);
}
- ConcurrentHashMap<String, List<Rule>> newRules = new ConcurrentHashMap<>(
+ Map<String, List<Rule>> newRules = new HashMap<>(
Review comment:
`new HashMap()` looks unnecessary.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] zhenxiao commented on a change in pull request #9302: Not
use for CoordinatorRuleManager.rules
Posted by GitBox <gi...@apache.org>.
zhenxiao commented on a change in pull request #9302: Not use for CoordinatorRuleManager.rules
URL: https://github.com/apache/druid/pull/9302#discussion_r375527247
##########
File path: server/src/main/java/org/apache/druid/server/router/CoordinatorRuleManager.java
##########
@@ -58,7 +57,7 @@
private final ObjectMapper jsonMapper;
private final Supplier<TieredBrokerConfig> config;
- private final AtomicReference<ConcurrentHashMap<String, List<Rule>>> rules;
+ private final AtomicReference<Map<String, List<Rule>>> rules;
Review comment:
thanks for your comments @suneet-s @jihoonson
updated to unmodifiableMap
I will file an issue to add tests after this PR is merged
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on issue #9302: Not use for
CoordinatorRuleManager.rules
Posted by GitBox <gi...@apache.org>.
jihoonson commented on issue #9302: Not use for CoordinatorRuleManager.rules
URL: https://github.com/apache/druid/pull/9302#issuecomment-581180370
@zhenxiao would you please update the title to be more accurate? Perhaps "Use HashMap instead of ConcurrentHashMap in CoordinatorRuleManager".
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #9302: Not
use for CoordinatorRuleManager.rules
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #9302: Not use for CoordinatorRuleManager.rules
URL: https://github.com/apache/druid/pull/9302#discussion_r375092308
##########
File path: server/src/main/java/org/apache/druid/server/router/CoordinatorRuleManager.java
##########
@@ -58,7 +57,7 @@
private final ObjectMapper jsonMapper;
private final Supplier<TieredBrokerConfig> config;
- private final AtomicReference<ConcurrentHashMap<String, List<Rule>>> rules;
+ private final AtomicReference<Map<String, List<Rule>>> rules;
Review 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.
@zhenxiao @suneet-s What do you think?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a change in pull request #9302: Not
use for CoordinatorRuleManager.rules
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #9302: Not use for CoordinatorRuleManager.rules
URL: https://github.com/apache/druid/pull/9302#discussion_r375404444
##########
File path: server/src/main/java/org/apache/druid/server/router/CoordinatorRuleManager.java
##########
@@ -58,7 +57,7 @@
private final ObjectMapper jsonMapper;
private final Supplier<TieredBrokerConfig> config;
- private final AtomicReference<ConcurrentHashMap<String, List<Rule>>> rules;
+ private final AtomicReference<Map<String, List<Rule>>> rules;
Review comment:
@jihoonson I'm not so familiar with this area of the code, so I'm happy to defer to your judgement.
@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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #9302: Use
HashMap for CoordinatorRuleManager.rules
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #9302: Use HashMap for CoordinatorRuleManager.rules
URL: https://github.com/apache/druid/pull/9302#discussion_r373867988
##########
File path: server/src/main/java/org/apache/druid/server/router/CoordinatorRuleManager.java
##########
@@ -80,7 +79,7 @@ public CoordinatorRuleManager(
this.druidLeaderClient = druidLeaderClient;
this.rules = new AtomicReference<>(
- new ConcurrentHashMap<String, List<Rule>>()
+ new HashMap<String, List<Rule>>()
Review comment:
Please use `Collections.emptyMap()`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson merged pull request #9302: Not use
ConcurrentHashMap in CoordinatorRuleManager.rules
Posted by GitBox <gi...@apache.org>.
jihoonson merged pull request #9302: Not use ConcurrentHashMap in CoordinatorRuleManager.rules
URL: https://github.com/apache/druid/pull/9302
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a change in pull request #9302: Not
use for CoordinatorRuleManager.rules
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #9302: Not use for CoordinatorRuleManager.rules
URL: https://github.com/apache/druid/pull/9302#discussion_r373869857
##########
File path: server/src/main/java/org/apache/druid/server/router/CoordinatorRuleManager.java
##########
@@ -58,7 +57,7 @@
private final ObjectMapper jsonMapper;
private final Supplier<TieredBrokerConfig> config;
- private final AtomicReference<ConcurrentHashMap<String, List<Rule>>> rules;
+ private final AtomicReference<Map<String, List<Rule>>> rules;
Review 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] zhenxiao commented on a change in pull request #9302: Use
HashMap for CoordinatorRuleManager.rules
Posted by GitBox <gi...@apache.org>.
zhenxiao commented on a change in pull request #9302: Use HashMap for CoordinatorRuleManager.rules
URL: https://github.com/apache/druid/pull/9302#discussion_r373868510
##########
File path: server/src/main/java/org/apache/druid/server/router/CoordinatorRuleManager.java
##########
@@ -149,7 +148,7 @@ public void poll()
);
}
- ConcurrentHashMap<String, List<Rule>> newRules = new ConcurrentHashMap<>(
+ Map<String, List<Rule>> newRules = new HashMap<>(
Review comment:
do you mean set rules directly, no need create newRules?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #9302: Not
use ConcurrentHashMap in CoordinatorRuleManager.rules
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #9302: Not use ConcurrentHashMap in CoordinatorRuleManager.rules
URL: https://github.com/apache/druid/pull/9302#discussion_r375646576
##########
File path: server/src/main/java/org/apache/druid/server/router/CoordinatorRuleManager.java
##########
@@ -58,7 +57,7 @@
private final ObjectMapper jsonMapper;
private final Supplier<TieredBrokerConfig> config;
- private final AtomicReference<ConcurrentHashMap<String, List<Rule>>> rules;
+ private final AtomicReference<Map<String, List<Rule>>> rules;
Review comment:
I raised https://github.com/apache/druid/pull/9318.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a change in pull request #9302: Not
use for CoordinatorRuleManager.rules
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #9302: Not use for CoordinatorRuleManager.rules
URL: https://github.com/apache/druid/pull/9302#discussion_r375404444
##########
File path: server/src/main/java/org/apache/druid/server/router/CoordinatorRuleManager.java
##########
@@ -58,7 +57,7 @@
private final ObjectMapper jsonMapper;
private final Supplier<TieredBrokerConfig> config;
- private final AtomicReference<ConcurrentHashMap<String, List<Rule>>> rules;
+ private final AtomicReference<Map<String, List<Rule>>> rules;
Review 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #9302: Not
use for CoordinatorRuleManager.rules
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #9302: Not use for CoordinatorRuleManager.rules
URL: https://github.com/apache/druid/pull/9302#discussion_r373872160
##########
File path: server/src/main/java/org/apache/druid/server/router/CoordinatorRuleManager.java
##########
@@ -58,7 +57,7 @@
private final ObjectMapper jsonMapper;
private final Supplier<TieredBrokerConfig> config;
- private final AtomicReference<ConcurrentHashMap<String, List<Rule>>> rules;
+ private final AtomicReference<Map<String, List<Rule>>> rules;
Review comment:
Good point. The value type (`List<Rule>`) should be immutable as well. I'm ok with using `UnmodifiableMap` and `UnmodifiableList`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #9302: Not
use for CoordinatorRuleManager.rules
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #9302: Not use for CoordinatorRuleManager.rules
URL: https://github.com/apache/druid/pull/9302#discussion_r373877374
##########
File path: server/src/main/java/org/apache/druid/server/router/CoordinatorRuleManager.java
##########
@@ -58,7 +57,7 @@
private final ObjectMapper jsonMapper;
private final Supplier<TieredBrokerConfig> config;
- private final AtomicReference<ConcurrentHashMap<String, List<Rule>>> rules;
+ private final AtomicReference<Map<String, List<Rule>>> rules;
Review 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #9302: Not
use for CoordinatorRuleManager.rules
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #9302: Not use for CoordinatorRuleManager.rules
URL: https://github.com/apache/druid/pull/9302#discussion_r375496361
##########
File path: server/src/main/java/org/apache/druid/server/router/CoordinatorRuleManager.java
##########
@@ -58,7 +57,7 @@
private final ObjectMapper jsonMapper;
private final Supplier<TieredBrokerConfig> config;
- private final AtomicReference<ConcurrentHashMap<String, List<Rule>>> rules;
+ private final AtomicReference<Map<String, List<Rule>>> rules;
Review 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #9302: Not
use for CoordinatorRuleManager.rules
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #9302: Not use for CoordinatorRuleManager.rules
URL: https://github.com/apache/druid/pull/9302#discussion_r373887130
##########
File path: server/src/main/java/org/apache/druid/server/router/CoordinatorRuleManager.java
##########
@@ -58,7 +57,7 @@
private final ObjectMapper jsonMapper;
private final Supplier<TieredBrokerConfig> config;
- private final AtomicReference<ConcurrentHashMap<String, List<Rule>>> rules;
+ private final AtomicReference<Map<String, List<Rule>>> rules;
Review comment:
@suneet-s on the second thought, I think you're right. I thought this PR simply changes the type of the map in the `rules` which is not the case. If this PR makes the map in the `rules` immutable, those tests should be added in this PR. Thanks for clarifying it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a change in pull request #9302: Not
use for CoordinatorRuleManager.rules
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #9302: Not use for CoordinatorRuleManager.rules
URL: https://github.com/apache/druid/pull/9302#discussion_r373872657
##########
File path: server/src/main/java/org/apache/druid/server/router/CoordinatorRuleManager.java
##########
@@ -58,7 +57,7 @@
private final ObjectMapper jsonMapper;
private final Supplier<TieredBrokerConfig> config;
- private final AtomicReference<ConcurrentHashMap<String, List<Rule>>> rules;
+ private final AtomicReference<Map<String, List<Rule>>> rules;
Review 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #9302: Use
HashMap for CoordinatorRuleManager.rules
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #9302: Use HashMap for CoordinatorRuleManager.rules
URL: https://github.com/apache/druid/pull/9302#discussion_r373867993
##########
File path: server/src/main/java/org/apache/druid/server/router/CoordinatorRuleManager.java
##########
@@ -120,7 +119,7 @@ public void stop()
return;
}
- rules.set(new ConcurrentHashMap<String, List<Rule>>());
+ rules.set(new HashMap<String, List<Rule>>());
Review comment:
Please use `Collections.emptyMap()`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org