You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@bookkeeper.apache.org by GitBox <gi...@apache.org> on 2022/06/23 16:18:22 UTC
[GitHub] [bookkeeper] horizonzy opened a new pull request, #3354: Resolve network location if the bookie is down but history holds it
horizonzy opened a new pull request, #3354:
URL: https://github.com/apache/bookkeeper/pull/3354
### Motivation
When resolve a bookie network location, only get from knownBookies. If the bookie shutdown, we can't resolve the network location. In fact, the history bookies maybe hold 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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] horizonzy commented on a diff in pull request #3354: Resolve network location if the bookie is down but history holds it
Posted by GitBox <gi...@apache.org>.
horizonzy commented on code in PR #3354:
URL: https://github.com/apache/bookkeeper/pull/3354#discussion_r905660276
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java:
##########
@@ -1026,7 +1026,14 @@ public PlacementPolicyAdherence isEnsembleAdheringToPlacementPolicy(List<BookieI
for (int j = 0; j < writeQuorumSize; j++) {
bookie = ensembleList.get((i + j) % ensembleSize);
try {
- racksInQuorum.add(knownBookies.get(bookie).getNetworkLocation());
+ BookieNode bookieNode = knownBookies.get(bookie);
+ if (bookieNode == null) {
+ bookieNode = historyBookies.get(bookie);
Review Comment:
Maybe I got it, if the bookie didn't active anymore, we didn't care the network location it is.
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] dlg99 commented on a diff in pull request #3354: Resolve network location if the bookie is down but history holds it
Posted by GitBox <gi...@apache.org>.
dlg99 commented on code in PR #3354:
URL: https://github.com/apache/bookkeeper/pull/3354#discussion_r906421404
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java:
##########
@@ -1026,7 +1026,14 @@ public PlacementPolicyAdherence isEnsembleAdheringToPlacementPolicy(List<BookieI
for (int j = 0; j < writeQuorumSize; j++) {
bookie = ensembleList.get((i + j) % ensembleSize);
try {
- racksInQuorum.add(knownBookies.get(bookie).getNetworkLocation());
+ BookieNode bookieNode = knownBookies.get(bookie);
+ if (bookieNode == null) {
+ bookieNode = historyBookies.get(bookie);
Review Comment:
Fix for NPE: https://github.com/apache/bookkeeper/pull/3350
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] horizonzy commented on a diff in pull request #3354: Resolve network location if the bookie is down but history holds it
Posted by GitBox <gi...@apache.org>.
horizonzy commented on code in PR #3354:
URL: https://github.com/apache/bookkeeper/pull/3354#discussion_r905626699
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java:
##########
@@ -1026,7 +1026,14 @@ public PlacementPolicyAdherence isEnsembleAdheringToPlacementPolicy(List<BookieI
for (int j = 0; j < writeQuorumSize; j++) {
bookie = ensembleList.get((i + j) % ensembleSize);
try {
- racksInQuorum.add(knownBookies.get(bookie).getNetworkLocation());
+ BookieNode bookieNode = knownBookies.get(bookie);
+ if (bookieNode == null) {
+ bookieNode = historyBookies.get(bookie);
Review Comment:
If the knownBookies didn't hold bookie, it will thown NPE. Can't resolve 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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] horizonzy closed pull request #3354: Resolve network location if the bookie is down but history holds it
Posted by GitBox <gi...@apache.org>.
horizonzy closed pull request #3354: Resolve network location if the bookie is down but history holds it
URL: https://github.com/apache/bookkeeper/pull/3354
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] horizonzy commented on a diff in pull request #3354: Resolve network location if the bookie is down but history holds it
Posted by GitBox <gi...@apache.org>.
horizonzy commented on code in PR #3354:
URL: https://github.com/apache/bookkeeper/pull/3354#discussion_r905631981
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java:
##########
@@ -1026,7 +1026,14 @@ public PlacementPolicyAdherence isEnsembleAdheringToPlacementPolicy(List<BookieI
for (int j = 0; j < writeQuorumSize; j++) {
bookie = ensembleList.get((i + j) % ensembleSize);
try {
- racksInQuorum.add(knownBookies.get(bookie).getNetworkLocation());
+ BookieNode bookieNode = knownBookies.get(bookie);
+ if (bookieNode == null) {
+ bookieNode = historyBookies.get(bookie);
Review Comment:
And the logic is a little bit strange, if we want to resolve bookie, the bookie must exist in the newest writeable bookies or history bookies. If there is a bookie exist in old ledger's ensembles, the bookie maybe not exist in the newest writeable bookies or history bookies, so I can't resolve the network location. It's unacceptable.
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] horizonzy commented on a diff in pull request #3354: Resolve network location if the bookie is down but history holds it
Posted by GitBox <gi...@apache.org>.
horizonzy commented on code in PR #3354:
URL: https://github.com/apache/bookkeeper/pull/3354#discussion_r905632570
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java:
##########
@@ -1026,7 +1026,14 @@ public PlacementPolicyAdherence isEnsembleAdheringToPlacementPolicy(List<BookieI
for (int j = 0; j < writeQuorumSize; j++) {
bookie = ensembleList.get((i + j) % ensembleSize);
try {
- racksInQuorum.add(knownBookies.get(bookie).getNetworkLocation());
+ BookieNode bookieNode = knownBookies.get(bookie);
+ if (bookieNode == null) {
+ bookieNode = historyBookies.get(bookie);
Review Comment:
I report it. #3355
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] dlg99 commented on a diff in pull request #3354: Resolve network location if the bookie is down but history holds it
Posted by GitBox <gi...@apache.org>.
dlg99 commented on code in PR #3354:
URL: https://github.com/apache/bookkeeper/pull/3354#discussion_r905357515
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java:
##########
@@ -1026,7 +1026,14 @@ public PlacementPolicyAdherence isEnsembleAdheringToPlacementPolicy(List<BookieI
for (int j = 0; j < writeQuorumSize; j++) {
bookie = ensembleList.get((i + j) % ensembleSize);
try {
- racksInQuorum.add(knownBookies.get(bookie).getNetworkLocation());
+ BookieNode bookieNode = knownBookies.get(bookie);
+ if (bookieNode == null) {
+ bookieNode = historyBookies.get(bookie);
Review Comment:
now the behavior depends on whether the bookie ran for awhile and hs history or freshly restarted.
AFAICT this should affect Auditor which does isEnsembleAdheringToPlacementPolicy but I guess our test coverage for the rackaware policy + bookie removal/Auditor runs is not good enough.
i think the only case where the historyBookies is to protect from DNS resolver errors, this changes expectations.
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org