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

[zookeeper] bump zookeeper to version 3.5.6 #5043

Merged
merged 4 commits into from Nov 1, 2019

Conversation

@addisonj
Copy link
Contributor

addisonj commented Aug 26, 2019

With zookeeper 3.5.5 now marked as stable and 3.5 being reccomend for
the production branch (see https://zookeeper.apache.org/releases.html),
this moves pulsar up to 3.5.6.

Pulsar previously had 3.5.x support but was downgraded due to concerns
of 3.5 still being a "beta", so this is a fairly simple change

Fixes #4448

As a follow on to this work, we should introduce new configuration options to support TLS connections to ZK, however, that is out of scope for this initial dependency bump.

Verifying this change

  • Make sure that the change passes the CI checks.

This change is already covered by existing tests

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): yes
  • The public API: no
  • The schema: no
  • The default values of configurations: no
  • The wire protocol: no
  • The rest endpoints: no
  • The admin cli options: no
  • Anything that affects deployment: yes

Documentation

  • Does this pull request introduce a new feature? no

There are some references to ZK 3.4 versions of docs that may need updated to 3.5 eventually. Assuming this is accepted, we should make a follow on item to change those

@merlimat merlimat added this to the 2.5.0 milestone Aug 26, 2019
@merlimat merlimat added the type/task label Aug 26, 2019
@merlimat

This comment has been minimized.

Copy link
Contributor

merlimat commented Aug 26, 2019

The ZK version should also be updated in the license file pulsar-sql/presto-distribution/LICENSE

@addisonj addisonj force-pushed the instructure:zk_3.5 branch from 249ed50 to eb77372 Aug 26, 2019
@aahmed-se aahmed-se assigned aahmed-se and addisonj and unassigned aahmed-se Aug 26, 2019
@addisonj

This comment has been minimized.

Copy link
Contributor Author

addisonj commented Aug 27, 2019

Any idea what is going on with unit tests here? It passes locally and seems like some jvms are just dieing but with no feedback

@merlimat

This comment has been minimized.

Copy link
Contributor

merlimat commented Aug 27, 2019

This seems a flaky one

@merlimat

This comment has been minimized.

Copy link
Contributor

merlimat commented Aug 27, 2019

run java8 tests

1 similar comment
@addisonj

This comment has been minimized.

Copy link
Contributor Author

addisonj commented Aug 27, 2019

run java8 tests

Copy link
Contributor

sijie left a comment

@merlimat @addisonj I remembered that there were some changes we made when downgrading zk to 3.4.x. Do we need to remove those hacks if there were (can't remember exactly what was the changes). There are problems in 3.5 when handling empty snapshots. I think we should clarify the situations in release notes or upgrade guide (we now have an upgrade guide, we should add the notes to it if it requires end-users' attentions).

@addisonj

This comment has been minimized.

Copy link
Contributor Author

addisonj commented Aug 28, 2019

I ran into the aforementioned upgrade issue with empty snapshots, but as I mentioned in PR, I had thought it might make sense to do a follow up for docs. As far as the hacks, I assume it may make sense to keep them around for some period of time for those who don't use embedded zookeeper and are sticking around 3.4?

This still seems to be failing tests as well (though they didn't run fine for locally...) but it seems to be pretty unlucky if it is just flaky.

@sijie

This comment has been minimized.

Copy link
Contributor

sijie commented Aug 28, 2019

I had thought it might make sense to do a follow up for docs.

Sorry that I missed your comment before. But since this is related to upgrade guide, can you please create a github issue and link the issue here? We have to mark that issue as a blocker for releasing 2.5.0.

I assume it may make sense to keep them around for some period of time for those who don't use embedded zookeeper and are sticking around 3.4?

I think the hack was done using AspectJ to inject deserializeTxn logic for handling a mixed transaction types between 3.4.x and 3.5.x. Hence I don't feel comfortable to have this hack logic applied to 3.5.x especially it is about deserializing transaction from zookeeper journals.

@eolivelli

This comment has been minimized.

Copy link
Contributor

eolivelli commented Aug 31, 2019

A fix for empty snapshot is coming for 3.5.6, see apache/zookeeper#1069

@aahmed-se

This comment has been minimized.

Copy link
Contributor

aahmed-se commented Sep 6, 2019

run java8 tests

2 similar comments
@aahmed-se

This comment has been minimized.

Copy link
Contributor

aahmed-se commented Sep 8, 2019

run java8 tests

@addisonj

This comment has been minimized.

Copy link
Contributor Author

addisonj commented Sep 16, 2019

run java8 tests

@addisonj

This comment has been minimized.

Copy link
Contributor Author

addisonj commented Sep 16, 2019

It seems like there might be more going on here with the failed tests? Any help in debugging this would be appreciated!

@addisonj addisonj force-pushed the instructure:zk_3.5 branch from f77b8c7 to 940b87f Sep 18, 2019
@addisonj

This comment has been minimized.

Copy link
Contributor Author

addisonj commented Sep 26, 2019

run integration test
run java8 tests
run cpp tests

@addisonj

This comment has been minimized.

Copy link
Contributor Author

addisonj commented Sep 26, 2019

run integration tests

@addisonj

This comment has been minimized.

Copy link
Contributor Author

addisonj commented Sep 27, 2019

rerun java8 tests
rerun integration tests

@addisonj

This comment has been minimized.

Copy link
Contributor Author

addisonj commented Sep 27, 2019

run java8 tests

@addisonj

This comment has been minimized.

Copy link
Contributor Author

addisonj commented Sep 27, 2019

run java8 tests
run integration tests

@addisonj

This comment has been minimized.

Copy link
Contributor Author

addisonj commented Sep 27, 2019

run java8 tests

1 similar comment
@aahmed-se

This comment has been minimized.

Copy link
Contributor

aahmed-se commented Sep 27, 2019

run java8 tests

@eolivelli

This comment has been minimized.

Copy link
Contributor

eolivelli commented Sep 27, 2019

What's the error?
It is not normal to need to re run tests so many times

@aahmed-se

This comment has been minimized.

Copy link
Contributor

aahmed-se commented Sep 27, 2019

@eolivelli jenkins has become unstable.

@addisonj

This comment has been minimized.

Copy link
Contributor Author

addisonj commented Sep 27, 2019

AFAICT, the errors are different each time and it isn't consistent. None of the tests (AFAICT) look to directly depend on ZK, but perhaps it has caused some stability issue? Hard to say, but seems quite weird

@addisonj

This comment has been minimized.

Copy link
Contributor Author

addisonj commented Oct 21, 2019

rerun java8 tests

@eolivelli

This comment has been minimized.

Copy link
Contributor

eolivelli commented Oct 21, 2019

Fyi now 3.5.6 is available :)

@merlimat

This comment has been minimized.

Copy link
Contributor

merlimat commented Oct 24, 2019

Fyi now 3.5.6 is available :)

Yes, and it includes fix for upgrading ZK with missing snapshot:
https://issues.apache.org/jira/browse/ZOOKEEPER-3056

@addisonj

@aahmed-se

This comment has been minimized.

Copy link
Contributor

aahmed-se commented Oct 25, 2019

@addisonj can you rebase this on master.

Addison Higham added 2 commits Aug 26, 2019
With zookeeper 3.5.x now marked as stable and 3.5 being reccomend for
the production branch (see https://zookeeper.apache.org/releases.html),
this moves pulsar up to 3.5.6.

Pulsar previously had 3.5.x support but was downgraded due to concerns
of 3.5 still being a "beta", so this is a fairly simple change

This also uses 3.5.6 as it has a fix for an issue where upgrades fail if
there is no snapshot files

Fixes #4448
Addison Higham
@addisonj addisonj force-pushed the instructure:zk_3.5 branch from 940b87f to d93dafb Oct 26, 2019
@addisonj addisonj changed the title [zookeeper] bump zookeeper to version 3.5.5 [zookeeper] bump zookeeper to version 3.5.6 Oct 26, 2019
@addisonj

This comment has been minimized.

Copy link
Contributor Author

addisonj commented Oct 26, 2019

@aahmed-se done

also I changed it to 3.5.6 as we want the fix for cleaner upgrades

@addisonj

This comment has been minimized.

Copy link
Contributor Author

addisonj commented Oct 28, 2019

rerun java8 tests

2 similar comments
@addisonj

This comment has been minimized.

Copy link
Contributor Author

addisonj commented Oct 28, 2019

rerun java8 tests

@addisonj

This comment has been minimized.

Copy link
Contributor Author

addisonj commented Oct 28, 2019

rerun java8 tests

@addisonj addisonj requested a review from sijie Oct 31, 2019
@addisonj

This comment has been minimized.

Copy link
Contributor Author

addisonj commented Oct 31, 2019

rerun java8 tests

@addisonj addisonj force-pushed the instructure:zk_3.5 branch from 3486a77 to 92ebc6f Nov 1, 2019
@addisonj

This comment has been minimized.

Copy link
Contributor Author

addisonj commented Nov 1, 2019

rerun java8 tests
rerun cpp tests

@aahmed-se

This comment has been minimized.

Copy link
Contributor

aahmed-se commented Nov 1, 2019

@sijie please take a look at unblock this.

@sijie
sijie approved these changes Nov 1, 2019
@sijie sijie merged commit 89a9aaa into apache:master Nov 1, 2019
3 checks passed
3 checks passed
Jenkins: C++ / Python Tests SUCCESS
Details
Jenkins: Integration Tests SUCCESS
Details
Jenkins: Java 8 - Unit Tests SUCCESS
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.