BP-41 Add flag to enable/disable BookieAddressResolver #3356
Conversation
massakam
force-pushed
the
disable-bookie-address-resolver
branch
3 times, most recently
from
d8a0184 to
1af2c49
Compare
StevenLuMT
left a comment
StevenLuMT
left a comment
There was a problem hiding this comment.
why change ServerConfiguration to ClientConfiguration in
tools/ledger/src/test/java/org/apache/bookkeeper/tools/cli/helpers/DiscoveryCommandTest.java ?
| * | ||
| * @return flag to enable/disable BookieAddressResolver. | ||
| */ | ||
| public boolean getEnableBookieAddressResolver() { |
There was a problem hiding this comment.
old code style is getBookieAddressResolverEnable
I think it's better
There was a problem hiding this comment.
@StevenLuMT Renamed to "getBookieAddressResolverEnabled". What do you think?
| * flag to enable/disable BookieAddressResolver. | ||
| * @return client configuration. | ||
| */ | ||
| public ClientConfiguration setEnableBookieAddressResolver(boolean enabled) { |
There was a problem hiding this comment.
setBookieAddressResolverEnable
There was a problem hiding this comment.
Renamed to "setBookieAddressResolverEnabled".
|
|
||
| this.serverConf = new ServerConfiguration(); | ||
| this.serverConf.setMetadataServiceUri("zk://127.0.0.1/path/to/ledgers"); | ||
| this.clientConf = new ClientConfiguration(); |
There was a problem hiding this comment.
why change ServerConfiguration to ClientConfiguration?
There was a problem hiding this comment.
@StevenLuMT The bookieAddressResolverEnabled added this time is a client-side setting, so it cannot be set for ServerConfiguration. I replaced ServerConfiguration with ClientConfiguration because I want to test the behavior of bookieAddressResolverEnabled when it is true and when it is false.
Also, I think that ClientConfiguration is more appropriate than ServerConfiguration to pass as the first argument of DiscoveryCommand#apply().
Even if ServerConfiguration is passed, it will be converted to ClientConfiguration internally.
massakam
force-pushed
the
disable-bookie-address-resolver
branch
from
1af2c49 to
c1f48bf
Compare
|
rerun failure checks |
|
PTAL |
eolivelli
left a comment
eolivelli
left a comment
There was a problem hiding this comment.
LGTM
I have left a little comment, please take a look
| * Resolve legacy style BookieIDs to Network addresses. | ||
| */ | ||
| @Slf4j | ||
| public class BookieAddressResolverDisabled implements BookieAddressResolver { |
There was a problem hiding this comment.
@eolivelli Added final modifier. Thanks.
### Motivation With BP-41, the BookKeeper client now needs a request to ZooKeeper to resolve the address from each bookie ID. In the following document, there is a description of the flag ~`enableBookieAddressResolver`~ `bookieAddressResolverEnabled` for disabling this feature by regarding the bookie ID as an address or hostname, but it seems that this has not been implemented yet. https://github.com/apache/bookkeeper/blob/master/site3/website/src/pages/bps/BP-41-bookieid.md I implemented this because I want this flag to reduce the number of requests to ZK. ### Changes Added a flag named ~`enableBookieAddressResolver`~ `bookieAddressResolverEnabled` to the client configuration. If this flag is false, use `BookieAddressResolverDisabled` instead of `DefaultBookieAddressResolver` as the address resolver for bookies. `BookieAddressResolverDisabled` regards a bookie ID to be in legacy format, i.e. "address:port" or "hostname:port", and returns the address of that bookie without access to ZK. Master Issue: #2396 (cherry picked from commit c31dff9)
### Motivation With BP-41, the BookKeeper client now needs a request to ZooKeeper to resolve the address from each bookie ID. In the following document, there is a description of the flag ~`enableBookieAddressResolver`~ `bookieAddressResolverEnabled` for disabling this feature by regarding the bookie ID as an address or hostname, but it seems that this has not been implemented yet. https://github.com/apache/bookkeeper/blob/master/site3/website/src/pages/bps/BP-41-bookieid.md I implemented this because I want this flag to reduce the number of requests to ZK. ### Changes Added a flag named ~`enableBookieAddressResolver`~ `bookieAddressResolverEnabled` to the client configuration. If this flag is false, use `BookieAddressResolverDisabled` instead of `DefaultBookieAddressResolver` as the address resolver for bookies. `BookieAddressResolverDisabled` regards a bookie ID to be in legacy format, i.e. "address:port" or "hostname:port", and returns the address of that bookie without access to ZK. Master Issue: apache#2396
6 participants
Motivation
With BP-41, the BookKeeper client now needs a request to ZooKeeper to resolve the address from each bookie ID.
In the following document, there is a description of the flag
enableBookieAddressResolverbookieAddressResolverEnabledfor disabling this feature by regarding the bookie ID as an address or hostname, but it seems that this has not been implemented yet.https://github.com/apache/bookkeeper/blob/master/site3/website/src/pages/bps/BP-41-bookieid.md
I implemented this because I want this flag to reduce the number of requests to ZK.
Changes
Added a flag named
enableBookieAddressResolverbookieAddressResolverEnabledto the client configuration. If this flag is false, useBookieAddressResolverDisabledinstead ofDefaultBookieAddressResolveras the address resolver for bookies.BookieAddressResolverDisabledregards a bookie ID to be in legacy format, i.e. "address:port" or "hostname:port", and returns the address of that bookie without access to ZK.Master Issue: #2396