Downgrading ZK to stable version 3.4.13 #2473
Conversation
There was a problem hiding this comment.
implementation lgtm.
However, this needs an integration test, that starts a 3.5 server, creates contains, and uses create2, then rolls back to ensure we can read the log.
Another alternative approach, though with more manual intervention, would be to make the user snapshot the zookeeper instance before upgrade. Could be ropey.
What about users which are sharing the zk instance for something else and are using the container stuff?
Sure. I tested manually so far. I have seen some It might be easy to add integration test that starts ZK from pulsar-2.1.0 docker image, writes some stuff and then upgrade to latest image with zk-3.4.x. Tricky part might be the zk-client dependency, but let's see.
Yes, since we're not actually depending on any of these new txn types, I wanted to have the smoothest downgrade path.
I wouldn't worry too much about this case of someone's using Pulsar bundled ZK and expecting it to be 3.5.x for some other applications. |
|
retest this please |
|
@ivankelly I have looked into adding integration test. The main problem I think is that since we're using docker based approach for int tests, I haven't found a way to get the container to write data on a mounted volume and then restart with same volume. |
|
@merlimat why do you need a mounted volume? You can have more than one version of the pulsar software on a single container, so start a 1.22 zookeeper, for example, write some data, shut it down, then start a 2.0 zookeeper on top of it. The test containers are all configured with supervisord. |
|
run java8 tests |
|
@ivankelly @sijie Added integration test with ZK 3.5 snapshot and log |
|
run java tests |
| for (BrokerContainer brokerContainer : pulsarCluster.getBrokers()) { | ||
| brokerContainer.withEnv("managedLedgerMaxEntriesPerLedger", String.valueOf(ENTRIES_PER_LEDGER)); | ||
| brokerContainer.withEnv("managedLedgerMinLedgerRolloverTimeMinutes", "0"); | ||
| brokerContainer.withEnv("managedLedgerOffloadDriver", "s3"); |
There was a problem hiding this comment.
offload stuff isn't needed.
| .numBrokers(1) | ||
| .clusterName(clusterName) | ||
| .classPathVolumeMounts( | ||
| ImmutableMap.<String, String> builder() |
There was a problem hiding this comment.
Is my understanding correct that this will be mounted by all containers?
There was a problem hiding this comment.
Yes, I left it open to not make it ZK specific. In case of other containers the zk data is simply ignored.
There was a problem hiding this comment.
Ok, could be a problem if testing multiple zk, but it's fine for now.
| .numBrokers(1) | ||
| .clusterName(clusterName) | ||
| .classPathVolumeMounts( | ||
| ImmutableMap.<String, String> builder() |
There was a problem hiding this comment.
The files in this path contain all the new operation types?
There was a problem hiding this comment.
Yes, added a couple missing now.
|
run java8 tests |
1 similar comment
|
run java8 tests |
|
@merlimat I think this change will break all the deployment scripts using |
|
It doesn't seem we're using |
* Downgrading ZK to stable version 3.4.13 * Added integration test for ZK server downgrade * Fixed ZookeeperClientFactoryImplTest.testZKCreationFailure * Fixed dependencies version in license file * Addressed comments * Fixed jline version in license file * There are 2 jline jars to count
Motivation
Ensure that we don't have dependency on a ZK version marked as "beta".
Modifications