You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@bookkeeper.apache.org by "TakaHiR07 (via GitHub)" <gi...@apache.org> on 2023/11/01 07:22:53 UTC
[PR] Fix RegionAwarePolicy can not update rackInfo between bookie left and join [bookkeeper]
TakaHiR07 opened a new pull request, #4122:
URL: https://github.com/apache/bookkeeper/pull/4122
### Motivation
When using regionAwarePolicy in pulsar, we encounter a case as following:
1. bookie shutdown, trigger handleBookiesThatLeft()
2. change bookie's rackInfo, trigger onBookieRackChange()
3. bookie start, trigger handleBookiesThatJoined()
However, the rackInfo is not updated actually, broker still write ensemble with old rackInfo.
### Changes
1. The Hashmap "address2Region" is local variable. It should be updated in handleBookiesThatJoined()
2. add a test to verify
--
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@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix RegionAwarePolicy can not update rackInfo between bookie left and join [bookkeeper]
Posted by "TakaHiR07 (via GitHub)" <gi...@apache.org>.
TakaHiR07 commented on code in PR #4122:
URL: https://github.com/apache/bookkeeper/pull/4122#discussion_r1429739627
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RegionAwareEnsemblePlacementPolicy.java:
##########
@@ -134,7 +134,8 @@ public void handleBookiesThatJoined(Set<BookieId> joinedBookies) {
topology.add(node);
knownBookies.put(addr, node);
historyBookies.put(addr, node);
- String region = getLocalRegion(node);
+ String region = parseBookieRegion(addr);
Review Comment:
I think current code has updated address2Region in onBookieRackChange(). If oldRegion == newRegion, no need to update. If oldRegion != newRegion, would put in newRegion.
https://github.com/apache/bookkeeper/blob/9aad0453f0f17f45ea673e82fb63762156315e8b/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RegionAwareEnsemblePlacementPolicy.java#L188-L194
--
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@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix RegionAwarePolicy can not update rackInfo between bookie left and join [bookkeeper]
Posted by "horizonzy (via GitHub)" <gi...@apache.org>.
horizonzy commented on code in PR #4122:
URL: https://github.com/apache/bookkeeper/pull/4122#discussion_r1429779876
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RegionAwareEnsemblePlacementPolicy.java:
##########
@@ -134,7 +134,8 @@ public void handleBookiesThatJoined(Set<BookieId> joinedBookies) {
topology.add(node);
knownBookies.put(addr, node);
historyBookies.put(addr, node);
- String region = getLocalRegion(node);
+ String region = parseBookieRegion(addr);
Review Comment:
I check the code. It's hard to get the rack info when the bookie is shutdown, because we can't resolve the address.
Your solution is good enough.
--
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@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix RegionAwarePolicy can not update rackInfo between bookie left and join [bookkeeper]
Posted by "horizonzy (via GitHub)" <gi...@apache.org>.
horizonzy commented on code in PR #4122:
URL: https://github.com/apache/bookkeeper/pull/4122#discussion_r1429741889
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RegionAwareEnsemblePlacementPolicy.java:
##########
@@ -134,7 +134,8 @@ public void handleBookiesThatJoined(Set<BookieId> joinedBookies) {
topology.add(node);
knownBookies.put(addr, node);
historyBookies.put(addr, node);
- String region = getLocalRegion(node);
+ String region = parseBookieRegion(addr);
Review Comment:
> I think current code has updated address2Region in onBookieRackChange(). If oldRegion == newRegion, no need to update. If oldRegion != newRegion, would put in newRegion.
>
> https://github.com/apache/bookkeeper/blob/9aad0453f0f17f45ea673e82fb63762156315e8b/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RegionAwareEnsemblePlacementPolicy.java#L188-L194
If the line_178 node is null, it does nothing.
--
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@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix RegionAwarePolicy can not update rackInfo between bookie left and join [bookkeeper]
Posted by "hangc0276 (via GitHub)" <gi...@apache.org>.
hangc0276 commented on PR #4122:
URL: https://github.com/apache/bookkeeper/pull/4122#issuecomment-1849400964
@horizonzy Would you please help take a look at this PR? Thanks.
--
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@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix RegionAwarePolicy can not update rackInfo between bookie left and join [bookkeeper]
Posted by "horizonzy (via GitHub)" <gi...@apache.org>.
horizonzy commented on code in PR #4122:
URL: https://github.com/apache/bookkeeper/pull/4122#discussion_r1429726177
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RegionAwareEnsemblePlacementPolicy.java:
##########
@@ -134,7 +134,8 @@ public void handleBookiesThatJoined(Set<BookieId> joinedBookies) {
topology.add(node);
knownBookies.put(addr, node);
historyBookies.put(addr, node);
- String region = getLocalRegion(node);
+ String region = parseBookieRegion(addr);
Review Comment:
Shall we update the `address2Region` in the `onBookieRackChange`.
Even if the bookie already has been shutdown, we can still update the `address2Region` in the `onBookieRackChange`
--
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@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix RegionAwarePolicy can not update rackInfo between bookie left and join [bookkeeper]
Posted by "TakaHiR07 (via GitHub)" <gi...@apache.org>.
TakaHiR07 commented on code in PR #4122:
URL: https://github.com/apache/bookkeeper/pull/4122#discussion_r1429751344
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RegionAwareEnsemblePlacementPolicy.java:
##########
@@ -134,7 +134,8 @@ public void handleBookiesThatJoined(Set<BookieId> joinedBookies) {
topology.add(node);
knownBookies.put(addr, node);
historyBookies.put(addr, node);
- String region = getLocalRegion(node);
+ String region = parseBookieRegion(addr);
Review Comment:
Is we need to get correct rack of a shutdown bookie ? It seem is enough to update bookie's rack once it restart.
--
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@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org