Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Entry write support local node region aware placement policy #4063

Conversation

hangc0276
Copy link
Contributor

Motivation

The entry write supports the local node Rack-Aware placement policy but does not support the local node Region-Aware placement policy
The entry read supports the local node Region-Aware placement policy but does not support the local node Rack-Aware placement policy
In order to match the local node region/rack awareness both on entry write and read, we need to support the following feature

Entry write supports local node region awareness placement policy
Entry read supports local node rack awareness placement policy

Modification

  • The PR aims to support the entry write local node region awareness placement policy.
  • It will select one bookie that is located in the same region and rack with the local node to form the ledger ensemble.

@hangc0276
Copy link
Contributor Author

@horizonzy Please help take a look at this PR, thanks.

bookie9Count++;
}

LOG.info("[hangc] ensemble: {}", ensemble);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can remove it.

LOG.info("Bookie1 Count: {}, Bookie8 Count: {}, Bookie9 Count: {}", bookie1Count, bookie8Count, bookie9Count);

//addr4 shutdown.
addrs.remove(addr5.toBookieId());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the purpose of this operation(remove addr5)

Copy link
Member

@horizonzy horizonzy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Copy link
Member

@zymap zymap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because it changes the current behaviors, do we need to allow it to configure?

@@ -388,9 +388,20 @@ public PlacementResult<List<BookieId>> newEnsemble(int ensembleSize, int writeQu
remainingEnsembleBeforeIteration = remainingEnsemble;
int regionsToAllocate = numRemainingRegions;
int startRegionIndex = lastRegionIndex % numRegionsAvailable;
int localRegionIndex = -1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a more clear way to do this is to remove the localRegion from the availableRegions then everything happen smoothly with original logic.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe not. If so, the localRegion node can't pick again. If we want to pick 5 nodes, and there are only two regions. If we remove localRegion, then we can only pick nodes from the remaining one region.

@merlimat merlimat merged commit 031069c into apache:master Jan 5, 2024
16 checks passed
Ghatage pushed a commit to sijie/bookkeeper that referenced this pull request Jul 12, 2024
…4063)

* Entry write support local node region aware placement policy

* address comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants