Skip to content

BP-41 Bookie Address changes tracking#2435

Merged
Ghatage merged 7 commits into
apache:masterfrom
eolivelli:fix/bp41-client-discovery-cache
Oct 20, 2020
Merged

BP-41 Bookie Address changes tracking#2435
Ghatage merged 7 commits into
apache:masterfrom
eolivelli:fix/bp41-client-discovery-cache

Conversation

@eolivelli

@eolivelli eolivelli commented Oct 10, 2020

Copy link
Copy Markdown
Contributor
  • Allow the client to follow Bookie Address Changes
  • If a Bookie changes network address the clients will be able to connect to the new addresess
  • In order to leverage the most of this feature the Bookie must configure a fixed bookieId (see BP-41)
  • add flag enableBookieAddressTracking to enable/disable this feature, as it needs a ZK watch for every active bookie in the cluster and if you have a static cluster there is no need to invalidate the local cache and issue more ZK requests
  • handle ZK session expiration events, when the ZK client is not receiving watch notifications it could miss bookie address changes, so we have to eagerly flush the whole local cache of bookie addresses

This is part of BP-41
Master ticket #2396

- follow Bookie Address Changes
- add flag enableBookieAddressTracking to enable/disable this feature
@eolivelli

eolivelli commented Oct 10, 2020

Copy link
Copy Markdown
Contributor Author

also @nicoloboschi @Ghatage PTAL

@eolivelli eolivelli changed the title BP-41 Follow Bookie Address changes tracking BP-41Bookie Address changes tracking Oct 12, 2020
@eolivelli eolivelli changed the title BP-41Bookie Address changes tracking BP-41 Bookie Address changes tracking Oct 12, 2020
@eolivelli

Copy link
Copy Markdown
Contributor Author

gently nudging at @sijie @rdhabalia @jiazhai

@Ghatage Ghatage left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Overall LGTM.
Can you please explain the upgrade scenario for this?
As I understand, after upgrading the bits we can still use the older bookie id but if we want, we can start the bookies with BP-41 enabled, and then we have to restart the clients with it enabled too? Is that right?

* flag to enable/disable bookie address changes tracking
* @return client configuration.
*/
public ClientConfiguration setEnableBookieAddressTracking(boolean enabled) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: We can use this to enable/disable, so not sure if naming variable as enabled is clear. It could just be value

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

changed

}

private class BookieServiceInfoCacheInvalidationWatcher implements Watcher {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How frequently can a bookieID change? What factors drive its change?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I see these known cases:

  • a bookie with a dynamic network address
  • changing configuration (change DNS name....)
  • you change configuration and add new endpoints (this will happen in the future, when for instance @sijie will contribute the pure TLS endpoint)

So basically I think they won't change very ofter

@eolivelli

Copy link
Copy Markdown
Contributor Author

Can you please explain the upgrade scenario for this?
As I understand, after upgrading the bits we can still use the older bookie id but if we want, we can start the bookies with BP-41 enabled, and then we have to restart the clients with it enabled too? Is that right?

If you don't configure the bookieId the bookie will work as it worked in 4.11, so no problem for old clients.
If you configure bookieId in your bookie then only 4.12 clients will be able to use it.

If you want to leverage Bookie Address Change Tracking you have to:

  • use bookidId
  • use 4.12
  • enable it in ClientConfiguration (it is enabled by default, in order to be friendly with bookies with dynamic addresses by default)

We cannot set a bookieId by default in order to be backward compatible

@Ghatage

Ghatage commented Oct 14, 2020

Copy link
Copy Markdown
Contributor

If you configure bookieId in your bookie then only 4.12 clients will be able to use it.

So can there be a case where we upgrade Bookies to 4.12 but disable this option and use the older hostname:port pairing
But then we won't be able to use 4.12 clients?
Sorry if I am misunderstanding this!

I don't see it as a problem, just wondering if we also need to call it out and update the documentation for this in the 4.12 release.

@eolivelli

Copy link
Copy Markdown
Contributor Author

If you simply upgrade an existing cluster from 4.9/10/11 to 4.12 everything will work well.
You can set enableBookieAddressTracking=false safely as bookies are using hostname:port as bookieId and it is not expected that this address changes.

I am setting enableBookieAddressTracking=true by default because I think this is the future for BK and also in order to have all tests working with this feature.

The case of enableBookieAddressTracking=false is to save resources on ZK, by reducing the number of watcher.

If we were on ZK 3.6.x (client + server) we could leverage "Persistent Recursive Watches", that is a single watch for an entire tree of znodes, without the need to set it again after it triggers, and this will also mitigate the impact of these new additional watches

@eolivelli

Copy link
Copy Markdown
Contributor Author

@jiazhai @rdhabalia Please help me merging this patch

@nicoloboschi nicoloboschi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

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.

3 participants