You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "rayluoluo (via GitHub)" <gi...@apache.org> on 2024/12/24 02:46:23 UTC
[I] [improve][broker] Improve the extensibility of the TopicBundleAssignmentStrategy interface class [pulsar]
rayluoluo opened a new issue, #23773:
URL: https://github.com/apache/pulsar/issues/23773
### Search before asking
- [X] I searched in the [issues](https://github.com/apache/pulsar/issues) and found nothing similar.
### Motivation
During the lookup process, the pulsar broker needs to determine the bundle to which the topic belongs, depending on the hash value calculated based on the topic name. After the [PIP-255](https://github.com/apache/pulsar/issues/19806) makes the partition assignment strategy pluggable, the process of obtaining the bundle triggered by the lookup is as follows:
`Lookup -> NamespaceService#getBrokerServiceUrlAsync -> NamespaceService#getBundleAsync ->
NamespaceBundles#findBundle -> TopicBundleAssignmentStrategy#findBundle -> NamespaceBundles#getBundle(long hash)`
When determining whether the current broker has the topic, the same hash algorithm needs to be used to calculate the hash value based on the topic name.
`NamespaceService#getOwnedTopicListForNamespaceBundle -> NamespaceBundle#includes -> NamespaceBundleFactory#getLongHashCode -> Range#contains(long hash)`
##
### Code bad smell: scatter-point modification
Unfortunately, in the current implementation of pulsar broker, the preceding two processes that need to calculate the hash value are implemented in different methods. Once the hash calculation method needs to be adjusted, the hash calculation method needs to be modified in different classes, which has a typical bad code smell problem.
The detailed description is as follows:
Take the default implementation class `ConsistentHashingTopicBundleAssigner` of the `TopicBundleAssignmentStrategy` interface class as an example. During the lookup process, the hash value is calculated in `ConsistentHashingTopicBundleAssigner#findBundle`.
https://github.com/apache/pulsar/blob/1967a9309586286580ac0f3b75a34e1f70e63f75/pulsar-broker/src/main/java/org/apache/pulsar/common/naming/ConsistentHashingTopicBundleAssigner.java#L25-L40
However, the process of determining whether the current broker has owned the topic is to calculate the hash value in the `NamespaceBundleFactory#getLongHashCode` method.
https://github.com/apache/pulsar/blob/1967a9309586286580ac0f3b75a34e1f70e63f75/pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java#L204
https://github.com/apache/pulsar/blob/1967a9309586286580ac0f3b75a34e1f70e63f75/pulsar-broker/src/main/java/org/apache/pulsar/common/naming/NamespaceBundleFactory.java#L78-L79
https://github.com/apache/pulsar/blob/1967a9309586286580ac0f3b75a34e1f70e63f75/pulsar-broker/src/main/java/org/apache/pulsar/common/naming/NamespaceBundleFactory.java#L294-L296
## Insufficient interface scalability
In addition, the `NamespaceBundleFactory#getLongHashCode` method uses a fixed algorithm to calculate the hash value. Therefore, it is difficult to extend other interface class implementations using different hash algorithms without modifying the `NamespaceBundleFactory#getLongHashCode` method, which violates the open and closed principles.
### Solution
It is recommended that the implementation of the `NamespaceBundleFactory#getLongHashCode` method be moved to the implementation class of the interface `TopicBundleAssignmentStrategy`. Therefore, we may add a new method `long getHashCode(String name)` to the `TopicBundleAssignmentStrategy` interface class. When the `NamespaceBundleFactory#getLongHashCode` method invokes `getHashCode`, `NamespaceBundles` is unaware of the specific hash algorithm implementation.
### Alternatives
_No response_
### Anything else?
_No response_
### Are you willing to submit a PR?
- [X] I'm willing to submit a PR!
--
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.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [improve][broker] Improve the extensibility of the TopicBundleAssignmentStrategy interface class [pulsar]
Posted by "merlimat (via GitHub)" <gi...@apache.org>.
merlimat commented on issue #23773:
URL: https://github.com/apache/pulsar/issues/23773#issuecomment-6388721525
Thanks @rayluoluo. The issue is real: lookups go through `TopicBundleAssignmentStrategy.findBundle`, while ownership checks (`NamespaceBundle.includes`) and admin paths use the fixed `NamespaceBundleFactory.getLongHashCode`, so a custom strategy can disagree with the broker. Fixing it means extending the plugin interface defined by PIP-255 and changing how every bundle-membership check hashes a topic. A plugin API change needs a PIP: https://github.com/apache/pulsar/blob/master/pip/README.md. Closing this for now. A PIP building on #23774 would be welcome.
--
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
Re: [I] [improve][broker] Improve the extensibility of the TopicBundleAssignmentStrategy interface class [pulsar]
Posted by "rayluoluo (via GitHub)" <gi...@apache.org>.
rayluoluo commented on issue #23773:
URL: https://github.com/apache/pulsar/issues/23773#issuecomment-2567511675
@315157973 @poorbarcode @liangyepianzhou @lhotari PTAL
--
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
Re: [I] [improve][broker] Improve the extensibility of the TopicBundleAssignmentStrategy interface class [pulsar]
Posted by "merlimat (via GitHub)" <gi...@apache.org>.
merlimat closed issue #23773: [improve][broker] Improve the extensibility of the TopicBundleAssignmentStrategy interface class
URL: https://github.com/apache/pulsar/issues/23773
--
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