[improve][broker] Handle get owned namespaces admin API in ExtensibleLoadManager #20552
Conversation
Demogorgon314
added
type/enhancement
Demogorgon314
requested review from
Technoboy-,
eolivelli,
gaoran10,
BewareMyPower and
mattisonchao
| @@ -1103,6 +1134,10 @@ public OwnershipCache getOwnershipCache() { | |||
| } | |||
|
|
|||
| public Set<NamespaceBundle> getOwnedServiceUnits() { | |||
| if (ExtensibleLoadManagerImpl.isLoadManagerExtensionEnabled(config)) { | |||
There was a problem hiding this comment.
Honestly, having all these references to a specific implementation (ExtensibleLoadManagerImpl) is a code smell.
We should rely on object oriented programming principals.
I am not going to block this patch, but I think that we are accumulating some tech debt that we will have to pay later
There was a problem hiding this comment.
Yes, we must do some refactoring in the feature... We might need to do some abstract for NamespaceService.
There was a problem hiding this comment.
It appears that the new vs old LM logic variations in NamespaceService are more than what we originally estimated. Yes, we can define NamespaceServiceExtension extends NamespaceService.
If the community decides to only maintain the extension logic in the future(I assume this wont happen in the near future), I think we can clean the old LMlogic in NamespaceService too.
| @@ -714,6 +734,12 @@ private void monitor() { | |||
| } | |||
| } | |||
|
|
|||
| public NamespaceBundle getNamespaceBundle(String bundle) { | |||
There was a problem hiding this comment.
Nit:move this to a static LoadManagerShared method.
| var stateData = entry.getValue(); | ||
| return stateData.state() == ServiceUnitState.Owned | ||
| && StringUtils.isNotBlank(stateData.dstBroker()) | ||
| && stateData.dstBroker().equals(brokerRegistry.getBrokerId()); |
There was a problem hiding this comment.
Nit: should we define a local var brokerId to avoid repeated registry access?
There was a problem hiding this comment.
Is repeated registry access will reduce performance?
There was a problem hiding this comment.
I think so. Afaik, the local variables will be likely in the CPU registers, which can be faster. brokerRegistry.getBrokerId() will need to deference, which could cause page hit/miss and stack getBrokerId() and deference again.
I am not sure if the modern java compiler optimizes this code with local var or not.
| @@ -1103,6 +1134,10 @@ public OwnershipCache getOwnershipCache() { | |||
| } | |||
|
|
|||
| public Set<NamespaceBundle> getOwnedServiceUnits() { | |||
| if (ExtensibleLoadManagerImpl.isLoadManagerExtensionEnabled(config)) { | |||
There was a problem hiding this comment.
It appears that the new vs old LM logic variations in NamespaceService are more than what we originally estimated. Yes, we can define NamespaceServiceExtension extends NamespaceService.
If the community decides to only maintain the extension logic in the future(I assume this wont happen in the near future), I think we can clean the old LMlogic in NamespaceService too.
Codecov Report
@@ Coverage Diff @@
## master #20552 +/- ##
=========================================
Coverage 72.94% 72.94%
Complexity 31930 31930
=========================================
Files 1867 1867
Lines 138561 138590 +29
Branches 15219 15222 +3
=========================================
+ Hits 101073 101097 +24
+ Misses 29466 29463 -3
- Partials 8022 8030 +8
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| */ | ||
| public Set<NamespaceBundle> getOwnedServiceUnits() { | ||
| var entrySet = serviceUnitStateChannel.getOwnershipEntrySet(); | ||
| var brokerId = brokerRegistry.getBrokerId(); |
There was a problem hiding this comment.
Is the var keyword necessary here?
While I don't have any objections to using the var keyword (I actually use it at work), it's worth considering the need for consistency in the codebase. As I haven't come across this usage in the project so far, it would be beneficial to reach a consensus on the usage of var.
There was a problem hiding this comment.
"As I haven't come across this usage in the project so far"
In fact, Pulsar already has been using "var".
There was a problem hiding this comment.
In fact, Pulsar already has been using "var".
Ah, is that so? Well, if that's the case, I will take back what I said 👍🏻 Thank you for letting me know.
But, may I ask in which class there is var usage? I also saw some in about 10 test classes, but not in production code yet. 🤔
There was a problem hiding this comment.
You can see the ServiceUnitStateChannelImpl class.
There was a problem hiding this comment.
@Demogorgon314 thanks🙏🏽
There was a problem hiding this comment.
I agree, we are not using "var" yet.
There was a problem hiding this comment.
IMO, It's ok to use var in the new features. Since we are already in the JDK17 and var will help avoid very long code causes us to have to break code into multiple lines to follow checkstyle rule.
For existing code, we should avoid using any new API&Feature to prevent getting a compile error when cherry-picking patches to previous branches.
Demogorgon314
deleted the
Demogorgon314/handle-get-owned-namespace-status-admin-api
branch
PIP: #16691
Motivation
The
ExtensibleLoadManagerdoes not handle theGetOwnedServiceUnitsmethod andGetOwnedNamespaceStatusmethod.Since the
ServiceUnitStateChannelwill store all the owner status, we need to filter all the other brokers when get owned service units to make the behavior the same asOwnershipCacheModifications
GetOwnedServiceUnitsmethod to get current broker ownership.ExtensibleLoadManagermake sure we handled all unloading calls.Documentation
docdoc-requireddoc-not-neededdoc-complete