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.

Sign up for GitHub

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

Jump to bottom

[Issue 9728] Upgrade GRPC and bookkeeper #9729

Merged
merged 9 commits into from
Mar 5, 2021
Merged

[Issue 9728] Upgrade GRPC and bookkeeper #9729

merged 9 commits into from
Mar 5, 2021

Conversation

dlg99
Copy link
Contributor

@dlg99 dlg99 commented Feb 25, 2021

Fixes #9728

Motivation

Upgrading dependencies to avoid dealing with deadlocks caused by old version of grpc

Modifications

Upgraded Bookkeeper and GRPC dependencies

Verifying this change

  • Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is a trivial rework / code cleanup without any test coverage.

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

eolivelli
eolivelli requested changes Feb 26, 2021
View reviewed changes
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

Very nice work!

Especially the work that has been done on BK in order to make BK stream storage stable, that is the foundation for Pulsar Stateful Function .

I left one comment please take a look

pom.xml
@@ -124,7 +124,7 @@ flexible messaging model and an intuitive client API.</description>
<typetools.version>0.5.0</typetools.version>
<protobuf3.version>3.11.4</protobuf3.version>
<protoc3.version>${protobuf3.version}</protoc3.version>
<grpc.version>1.18.0</grpc.version>
<grpc.version>1.33.0</grpc.version>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we jump to 1.36.0 ?

I known that in BK we stick to 1.33 but IIRC it is due to the etcd client. In Pulsar we do not have an etcd client.
As it will still take more time to test 2.8 before a release, we could upgrade to latest GRPC version now and have all of the time to see if it is working well with Pulsar

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eolivelli Etcd client broke because grpc 1.36 depends on netty 1.52.
From what I understood, the culprit was was this change netty/netty#10407 in 4.1.52 that got reverted netty/netty#10980
The change got reverted because TLS_FALSE_START slightly changes the "flow" during handshake which may cause suprises for the end-user
We can try mixing netty 1.51 (or 1.59, where the change is supposedly reverted, it was not out when I tested bookkeeper) with grpc 1.36 but I am not sure how much I can rely on existing tests (and many of them don't pass currently anyways, with or without my changes). I'd very much prefer not to experiment with this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fine to me

Copy link
Member

Choose a reason for hiding this comment

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

It would be useful to switch to use gprc-bom also in Pulsar, in the similar way as it was done for Bookkeeper: https://github.com/apache/bookkeeper/blob/7327e3b1c636ed42bdbe231fc30bcb73e527fee8/pom.xml#L462-L468 .
gprc-bom became available in 1.25 and that's why it hasn't been used before.

eolivelli
eolivelli approved these changes Mar 2, 2021
View reviewed changes
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

LGTM
Nice work

lhotari
lhotari requested changes Mar 4, 2021
View reviewed changes
pom.xml
@@ -124,7 +124,7 @@ flexible messaging model and an intuitive client API.</description>
<typetools.version>0.5.0</typetools.version>
<protobuf3.version>3.11.4</protobuf3.version>
<protoc3.version>${protobuf3.version}</protoc3.version>
<grpc.version>1.18.0</grpc.version>
<grpc.version>1.33.0</grpc.version>
Copy link
Member

Choose a reason for hiding this comment

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

It would be useful to switch to use gprc-bom also in Pulsar, in the similar way as it was done for Bookkeeper: https://github.com/apache/bookkeeper/blob/7327e3b1c636ed42bdbe231fc30bcb73e527fee8/pom.xml#L462-L468 .
gprc-bom became available in 1.25 and that's why it hasn't been used before.

@codelipenghui codelipenghui added this to the 2.8.0 milestone Mar 5, 2021
@merlimat merlimat merged commit c0a9ba3 into apache:master Mar 5, 2021
mlyahmed pushed a commit to mlyahmed/pulsar that referenced this pull request Mar 5, 2021
@dlg99 @mlyahmed
[Issue 9728] Upgrade GRPC and bookkeeper (apache#9729)
691b842 
* Upgraded bookkeeper to 4.13.0; grpc to 1.33

* updated LICENSE

* fixed the rest of the license check

* no idea why license check results on CI are different from local :/

* one more license fix

* Integration tests failed to start container/service there because of java.lang.NoClassDefFoundError: io/perfmark/PerfMark

* another try to get the permark.jar in

* another take on perfmark.jar and integration tests

* add perfmark.jar to LICENSE
@dlg99 dlg99 deleted the master-grpc branch October 14, 2021 23:29
dlg99 added a commit to dlg99/pulsar that referenced this pull request Dec 1, 2021
@dlg99
[Issue 9728] Upgrade GRPC and bookkeeper (apache#9729)
3cc6aaf 
* Upgraded bookkeeper to 4.13.0; grpc to 1.33

* updated LICENSE

* fixed the rest of the license check

* no idea why license check results on CI are different from local :/

* one more license fix

* Integration tests failed to start container/service there because of java.lang.NoClassDefFoundError: io/perfmark/PerfMark

* another try to get the permark.jar in

* another take on perfmark.jar and integration tests

* add perfmark.jar to LICENSE
eolivelli pushed a commit to datastax/pulsar that referenced this pull request Dec 1, 2021
@dlg99 @hangc0276
@zymap @merlimat
Grpc upgrade; BK upgrade for LS 2.7 (#14)
116f90e 
* [Issue 9728] Upgrade GRPC and bookkeeper (apache#9729)

* Upgraded bookkeeper to 4.13.0; grpc to 1.33

Fixes apache#10937

*Motivation*

The previours bk version was introduce an incompatible issue
with BouncyCastle FIPS. BookKeeper already fix this and we
should upgrade the version to resolve the issue.

For more information: apache#10937

*Modifications*

* Upgrade to BookKeeper 4.14.3 (apache#12760)

* Fixed additional post-cherry-pick conflicts

Co-authored-by: hangc0276 <chenhang@apache.org>
Co-authored-by: chenhang <chenhang@bigo.sg>
Co-authored-by: Yong Zhang <zhangyong1025.zy@gmail.com>
Co-authored-by: Matteo Merli <mmerli@apache.org>
Jason918 pushed a commit to Jason918/pulsar that referenced this pull request Aug 4, 2022
@dlg99 @Jason918
[Issue 9728] Upgrade GRPC and bookkeeper (apache#9729)
3b3b5a1 
* Upgraded bookkeeper to 4.13.0; grpc to 1.33

* updated LICENSE

* fixed the rest of the license check

* no idea why license check results on CI are different from local :/

* one more license fix

* Integration tests failed to start container/service there because of java.lang.NoClassDefFoundError: io/perfmark/PerfMark

* another try to get the permark.jar in

* another take on perfmark.jar and integration tests

* add perfmark.jar to LICENSE
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lhotari lhotari lhotari requested changes

@codelipenghui codelipenghui codelipenghui approved these changes

@eolivelli eolivelli eolivelli approved these changes

@merlimat merlimat merlimat approved these changes

Assignees

@dlg99 dlg99

Labels
None yet
Projects
None yet
Milestone
2.8.0
Development

Successfully merging this pull request may close these issues.

Upgrade GRPC and bookkeeper
5 participants
@dlg99 @merlimat @lhotari @eolivelli @codelipenghui