ZOOKEEPER-4936 - Make ZookeeperServer.shutdown(fullyShutdown) Non-final fix curator's TestingZookeeperMain

[vinayakumarb]

No description provided.

[ctubbsii]

The change seems simple enough, and beneficial, but do you know the justification for making it final in the first place? Will this introduce some regression because it fails to consider that original justification?

[kezhuw]

Hi @vinayakumarb, does apache/curator#1252 solves your concerns ?

do you know the justification for making it final in the first place?

I think it is because both void shutdown() and void shutdown(boolean fullyShutDown) are open to be overrode before the fix bc9afbf to efbd660 which calls shutdown(boolean fullyShutDown) directly and bypass overrides in shutdown().

[vinayakumarb]

The change seems simple enough, and beneficial, but do you know the justification for making it final in the first place? Will this introduce some regression because it fails to consider that original justification?

There was no justification provided while making it final in the commit.

This change as been done in a subminor version release and broken Curator's TestingZookeeperMain class. Because of this, upgrading to zookeeper 3.9.3 is blocked, which breaks the tests which are using TestingZookeeperMain from curator.

in general, breaking changes are not advised in a subminor version. I totally agree that, shutdown() is not an API, but this change stops from adoption immediately.

Change apache/curator#1252 would solve the problem. But that needs a release from Curator and down streams needs to adopt that as well which are not as frequent as zookeeper.

[kezhuw]

breaking changes are not advised in a subminor version. I totally agree that, shutdown() is not an API, but this change stops from adoption immediately.

ZooKeeperServerEmbedded was designed to be public in mind. Curator has migrated to it but it still have source code level dependencies on ZooKeeperServer.

There was no justification provided while making it final in the commit.

My cents:

1. void shutdown() was changed to final to fix ZOOKEEPER-4712.
2. I would like void shutdown(boolean fullyShutDown) to be internal usages.
3. void shutdownComponents() should be used instead above two.

Change apache/curator#1252 would solve the problem. But that needs a release from Curator and down streams needs to adopt that as well which are not as frequent as zookeeper.

This is a compilation error, people will have to upgrade either curator or zookeeper anyway. I started a discuss to release a curator version for this: https://lists.apache.org/thread/jywgcrk11whh8km7q6btc6k574vx1lrq

Given above, it would be -0 from my side to make void shutdown(boolean fullyShutDown) non final.