You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/09/09 14:37:56 UTC
[GitHub] [pulsar] coderzc opened a new pull request, #17575: [cleanup][broker] Use set instead of list to improve `contains`
coderzc opened a new pull request, #17575:
URL: https://github.com/apache/pulsar/pull/17575
### Motivation
Now, `AVAILABLE_ALGORITHMS` and `supportedNamespaceBundleSplitAlgorithms` use list to create, we can use set to improve `contains`.
https://github.com/apache/pulsar/blob/05a2ea87371783000f496ce8103c0ac372e4a8fe/pulsar-broker/src/main/java/org/apache/pulsar/PulsarBrokerStarter.java#L177-L188
### Modifications
Use set instead of list.
### Documentation
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `doc-required`
(Your PR needs to update docs and you will update later)
- [x] `doc-not-needed`
(Please explain why)
- [ ] `doc`
(Your PR contains doc changes)
- [ ] `doc-complete`
(Docs have been already added)
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tisonkun commented on a diff in pull request #17575: [cleanup][broker] Use set instead of list to improve `contains` and `containsAll` performance.
Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #17575:
URL: https://github.com/apache/pulsar/pull/17575#discussion_r967232293
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java:
##########
@@ -2337,7 +2336,7 @@ public class ServiceConfiguration implements PulsarConfiguration {
category = CATEGORY_LOAD_BALANCER,
doc = "Supported algorithms name for namespace bundle split"
)
- private List<String> supportedNamespaceBundleSplitAlgorithms = Lists.newArrayList("range_equally_divide",
+ private Set<String> supportedNamespaceBundleSplitAlgorithms = Sets.newHashSet("range_equally_divide",
Review Comment:
I'd prefer to do so since #16991 is ongoing. @youzipi & @MarvinCai may investigate why there's still such remaining.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] coderzc commented on a diff in pull request #17575: [cleanup][broker] Use set instead of list to improve `contains` and `containsAll` performance.
Posted by GitBox <gi...@apache.org>.
coderzc commented on code in PR #17575:
URL: https://github.com/apache/pulsar/pull/17575#discussion_r967225355
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java:
##########
@@ -2337,7 +2336,7 @@ public class ServiceConfiguration implements PulsarConfiguration {
category = CATEGORY_LOAD_BALANCER,
doc = "Supported algorithms name for namespace bundle split"
)
- private List<String> supportedNamespaceBundleSplitAlgorithms = Lists.newArrayList("range_equally_divide",
+ private Set<String> supportedNamespaceBundleSplitAlgorithms = Sets.newHashSet("range_equally_divide",
Review Comment:
No, I base on newest master. See:
https://github.com/apache/pulsar/blob/master/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java#L2335-L2341
It seems `modernize` only banned the methods with no parameters.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] coderzc commented on a diff in pull request #17575: [cleanup][broker] Use set instead of list to improve `contains` and `containsAll` performance.
Posted by GitBox <gi...@apache.org>.
coderzc commented on code in PR #17575:
URL: https://github.com/apache/pulsar/pull/17575#discussion_r967241412
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java:
##########
@@ -2337,7 +2336,7 @@ public class ServiceConfiguration implements PulsarConfiguration {
category = CATEGORY_LOAD_BALANCER,
doc = "Supported algorithms name for namespace bundle split"
)
- private List<String> supportedNamespaceBundleSplitAlgorithms = Lists.newArrayList("range_equally_divide",
+ private Set<String> supportedNamespaceBundleSplitAlgorithms = Sets.newHashSet("range_equally_divide",
Review Comment:
I found still keep a lot `Sets.newHashSet(...)` in the #17275. And the `Set.of()` create an unmodifiable set, so them is not the same.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] coderzc commented on a diff in pull request #17575: [cleanup][broker] Use set instead of list to improve `contains` and `containsAll` performance.
Posted by GitBox <gi...@apache.org>.
coderzc commented on code in PR #17575:
URL: https://github.com/apache/pulsar/pull/17575#discussion_r967225355
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java:
##########
@@ -2337,7 +2336,7 @@ public class ServiceConfiguration implements PulsarConfiguration {
category = CATEGORY_LOAD_BALANCER,
doc = "Supported algorithms name for namespace bundle split"
)
- private List<String> supportedNamespaceBundleSplitAlgorithms = Lists.newArrayList("range_equally_divide",
+ private Set<String> supportedNamespaceBundleSplitAlgorithms = Sets.newHashSet("range_equally_divide",
Review Comment:
No, I base on master. See:
https://github.com/apache/pulsar/blob/master/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java#L2335-L2341
I should be change it to `Set.of()` ?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] coderzc commented on a diff in pull request #17575: [cleanup][broker] Use set instead of list to improve `contains` and `containsAll` performance.
Posted by GitBox <gi...@apache.org>.
coderzc commented on code in PR #17575:
URL: https://github.com/apache/pulsar/pull/17575#discussion_r967241412
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java:
##########
@@ -2337,7 +2336,7 @@ public class ServiceConfiguration implements PulsarConfiguration {
category = CATEGORY_LOAD_BALANCER,
doc = "Supported algorithms name for namespace bundle split"
)
- private List<String> supportedNamespaceBundleSplitAlgorithms = Lists.newArrayList("range_equally_divide",
+ private Set<String> supportedNamespaceBundleSplitAlgorithms = Sets.newHashSet("range_equally_divide",
Review Comment:
I found still keep a lot `Sets.newHashSet(...)` in the #17275.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] coderzc closed pull request #17575: [cleanup][broker] Use set instead of list to improve `contains` and `containsAll` performance.
Posted by GitBox <gi...@apache.org>.
coderzc closed pull request #17575: [cleanup][broker] Use set instead of list to improve `contains` and `containsAll` performance.
URL: https://github.com/apache/pulsar/pull/17575
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] coderzc commented on a diff in pull request #17575: [cleanup][broker] Use set instead of list to improve `contains` and `containsAll` performance.
Posted by GitBox <gi...@apache.org>.
coderzc commented on code in PR #17575:
URL: https://github.com/apache/pulsar/pull/17575#discussion_r967225355
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java:
##########
@@ -2337,7 +2336,7 @@ public class ServiceConfiguration implements PulsarConfiguration {
category = CATEGORY_LOAD_BALANCER,
doc = "Supported algorithms name for namespace bundle split"
)
- private List<String> supportedNamespaceBundleSplitAlgorithms = Lists.newArrayList("range_equally_divide",
+ private Set<String> supportedNamespaceBundleSplitAlgorithms = Sets.newHashSet("range_equally_divide",
Review Comment:
No, I base on newest master. See:
https://github.com/apache/pulsar/blob/master/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java#L2335-L2341
I should be change it to Set.of() ? It seems `modernize` only banned the methods with no parameters.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] coderzc commented on a diff in pull request #17575: [cleanup][broker] Use set instead of list to improve `contains` and `containsAll` performance.
Posted by GitBox <gi...@apache.org>.
coderzc commented on code in PR #17575:
URL: https://github.com/apache/pulsar/pull/17575#discussion_r967241412
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java:
##########
@@ -2337,7 +2336,7 @@ public class ServiceConfiguration implements PulsarConfiguration {
category = CATEGORY_LOAD_BALANCER,
doc = "Supported algorithms name for namespace bundle split"
)
- private List<String> supportedNamespaceBundleSplitAlgorithms = Lists.newArrayList("range_equally_divide",
+ private Set<String> supportedNamespaceBundleSplitAlgorithms = Sets.newHashSet("range_equally_divide",
Review Comment:
I found still keep a lot `Sets.newHashSet(...)` in the https://github.com/apache/pulsar/issues/16991.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] coderzc commented on pull request #17575: [cleanup][broker] Use set instead of list to improve `contains` and `containsAll` performance.
Posted by GitBox <gi...@apache.org>.
coderzc commented on PR #17575:
URL: https://github.com/apache/pulsar/pull/17575#issuecomment-1242590573
> LGTM. I'd like to know whether this code change is on the hot path (critical path).
>
> That is, if we make minor improvements outside hot paths, it's OK but not very valuable. If you made the change on a critical path and stated a performance improvement, is there any (micro) performance report for the diff?
This is just a minor improvement, I think it is a good programming practice.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tisonkun commented on a diff in pull request #17575: [cleanup][broker] Use set instead of list to improve `contains` and `containsAll` performance.
Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #17575:
URL: https://github.com/apache/pulsar/pull/17575#discussion_r967212043
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java:
##########
@@ -2337,7 +2336,7 @@ public class ServiceConfiguration implements PulsarConfiguration {
category = CATEGORY_LOAD_BALANCER,
doc = "Supported algorithms name for namespace bundle split"
)
- private List<String> supportedNamespaceBundleSplitAlgorithms = Lists.newArrayList("range_equally_divide",
+ private Set<String> supportedNamespaceBundleSplitAlgorithms = Sets.newHashSet("range_equally_divide",
Review Comment:
@coderzc are you on an old base branch? @MarvinCai IIRC we enable modernize plugin for `pulsar-broker-common` module and this usage should be banned.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] coderzc commented on a diff in pull request #17575: [cleanup][broker] Use set instead of list to improve `contains` and `containsAll` performance.
Posted by GitBox <gi...@apache.org>.
coderzc commented on code in PR #17575:
URL: https://github.com/apache/pulsar/pull/17575#discussion_r967225355
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java:
##########
@@ -2337,7 +2336,7 @@ public class ServiceConfiguration implements PulsarConfiguration {
category = CATEGORY_LOAD_BALANCER,
doc = "Supported algorithms name for namespace bundle split"
)
- private List<String> supportedNamespaceBundleSplitAlgorithms = Lists.newArrayList("range_equally_divide",
+ private Set<String> supportedNamespaceBundleSplitAlgorithms = Sets.newHashSet("range_equally_divide",
Review Comment:
No, I base on newest master. See:
https://github.com/apache/pulsar/blob/master/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java#L2335-L2341
I should be change it to `Set.of()` ?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] HQebupt commented on pull request #17575: [cleanup][broker] Use set instead of list to improve `contains` and `containsAll` performance.
Posted by GitBox <gi...@apache.org>.
HQebupt commented on PR #17575:
URL: https://github.com/apache/pulsar/pull/17575#issuecomment-1242753858
> LGTM. I'd like to know whether this code change is on the hot path (critical path).
>
> That is, if we make minor improvements outside hot paths, it's OK but not very valuable. If you made the change on a critical path and stated a performance improvement, is there any (micro) performance report for the diff?
I agree with @tisonkun . The `Set` occupies more memory space than `List`. The performance improvement is small, when the data is small. LGTM.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tisonkun commented on a diff in pull request #17575: [cleanup][broker] Use set instead of list to improve `contains` and `containsAll` performance.
Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #17575:
URL: https://github.com/apache/pulsar/pull/17575#discussion_r967554873
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java:
##########
@@ -2337,7 +2336,7 @@ public class ServiceConfiguration implements PulsarConfiguration {
category = CATEGORY_LOAD_BALANCER,
doc = "Supported algorithms name for namespace bundle split"
)
- private List<String> supportedNamespaceBundleSplitAlgorithms = Lists.newArrayList("range_equally_divide",
+ private Set<String> supportedNamespaceBundleSplitAlgorithms = Sets.newHashSet("range_equally_divide",
Review Comment:
> create an unmodifiable set
That's it! I think JDK provides only the unmodifiable set version and `new HashSet<>()` which doesn't accept an element parameter. Thanks for your investigation!
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tisonkun commented on a diff in pull request #17575: [cleanup][broker] Use set instead of list to improve `contains` and `containsAll` performance.
Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #17575:
URL: https://github.com/apache/pulsar/pull/17575#discussion_r967232293
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java:
##########
@@ -2337,7 +2336,7 @@ public class ServiceConfiguration implements PulsarConfiguration {
category = CATEGORY_LOAD_BALANCER,
doc = "Supported algorithms name for namespace bundle split"
)
- private List<String> supportedNamespaceBundleSplitAlgorithms = Lists.newArrayList("range_equally_divide",
+ private Set<String> supportedNamespaceBundleSplitAlgorithms = Sets.newHashSet("range_equally_divide",
Review Comment:
I'd prefer to do so since #12271 is ongoing. @youzipi & @MarvinCai may investigate why there's still such remaining.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] coderzc commented on pull request #17575: [cleanup][broker] Use set instead of list to improve `contains` and `containsAll` performance.
Posted by GitBox <gi...@apache.org>.
coderzc commented on PR #17575:
URL: https://github.com/apache/pulsar/pull/17575#issuecomment-1242834504
> The `Set` occupies more memory space than `List`.
It makes sense. I close 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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org