Upgrading GRPC version to 1.33, Netty to 4.1.50Final and ETCD client driver by dlg99 · Pull Request #2582 · apache/bookkeeper

dlg99 · 2021-02-03T23:51:32Z

Descriptions of the changes in this PR:

upgraded GRPC
Will need to upgrade GRPC in pulsar as well

Upgrading:

GRPC version to 1.33
Netty to 4.1.50Final
ETCD client driver
TestContainers

Motivation

TableService stopped working under load.
jstack points to a problem with GRPC that is fixed in later versions

Changes

upgraded GRPC

Master Issue: #2581

In order to uphold a high standard for quality for code contributions, Apache BookKeeper runs various precommit
checks for pull requests. A pull request can only be merged when it passes precommit checks.

Be sure to do all of the following to help us incorporate your contribution
quickly and easily:

If this PR is a BookKeeper Proposal (BP):

Make sure the PR title is formatted like:
<BP-#>: Description of bookkeeper proposal
e.g. BP-1: 64 bits ledger is support

Attach the master issue link in the description of this PR.

Attach the google doc link if the BP is written in Google Doc.

Otherwise:

Make sure the PR title is formatted like:
<Issue #>: Description of pull request
e.g. Issue 123: Description ...

Make sure tests pass via mvn clean apache-rat:check install spotbugs:check.

Replace <Issue #> in the title with the actual Issue number.

nicoloboschi

+1

eolivelli

LGTM

but unfortunately Integration tests of the Etcd metadata driver are failing```

Errors:
Error: org.apache.bookkeeper.metadata.etcd.Etcd64bitIdGeneratorTest.testGenerateIdParallel
Error: Run 1: Etcd64bitIdGeneratorTest.setUp:57->EtcdTestBase.setUp:79->EtcdTestBase.newEtcdClient:61 » NoSuchMethod
Error: Run 2: Etcd64bitIdGeneratorTest.setUp:57->EtcdTestBase.setUp:79->EtcdTestBase.newEtcdClient:61 » NoSuchMethod
Error: Run 3: Etcd64bitIdGeneratorTest.setUp:57->EtcdTestBase.setUp:79->EtcdTestBase.newEtcdClient:61 » NoSuchMethod
[INFO]

lhotari · 2021-02-04T08:29:05Z

    <google.errorprone.version>2.1.2</google.errorprone.version>
-    <grpc.version>1.18.0</grpc.version>
+    <grpc.version>1.35.0</grpc.version>
+    <tomcat-annotations.version>1.35.0</tomcat-annotations.version>


I guess that the tomcat-annotations.version is not correct. What is the exact reason for introducing this dependency?
It looks like the dependency isn't maintained, updated last in 2017: https://search.maven.org/artifact/org.apache.tomcat/annotations-api

This is resolved.

dlg99 · 2021-02-04T22:05:25Z

The problem with the integration tests seems to be related to the etcd client.
We use com.coreos:jetcd-core 0.0.2 currently, it is 2 years old and unmaintained. It relies on the API that no longer exists in newer grpc
The new etcd client is io.etcd:jetcd-core 0.5.4; all imports and some APIs seem to have change as well; I'll try to update etcd client as well.

I am not sure what/whom this can affect but anyone running 2yo etcd should consider rethinking their life choices. :/

…update for metadata-drivers

…de grpc a little (to 1.33) to get integration tests to work

dlg99 · 2021-02-08T18:57:35Z

So far:
I had to upgrade etcd client, which dragged testcontainers (and docker-java) upgrade as well.
All tests pass now.

I left netty at its current version. jetcd dependencies use newer version but TLS tests timeout with it
I see that netty had this change netty/netty#10407 in 4.1.52 that got reverted netty/netty#10980 in 4.1.59 (today's release, not even on maven central yet) - I suspect this is the reason for the test timeout. As long as etcd test pass I assume it is ok with older version.

A few notes:
Most of the API changes (deprecated => current) are rather mechanical.
EtcdBookieRegister deserves closer look.
I left deprecated grpc API in couple of places (i.e. BKRegistrationNameResolverProvider) because one experimental API (ManagedChannelBuilder.nameResolverFactory) is deprecated in favor of another experimental API (NameResolverRegistry) and introducing larger changes didn't make sense if that experimental change can get deprecated soon anyways.

eolivelli

Great work.
I will check tomorrow the hardest parts.

Aren't we updating LICENSE files?

dlg99 · 2021-02-09T01:13:41Z

@eolivelli The licenses weren't updated in a while. I.e. licenses still referred netty 4.1.32, the build is on 4.1.50, similarly others (rocksdb etc).
I did first pass on updating the licenses for the server, haven't touched two other tars.
TBH at this point I am thinking that the best way to deal with this is:

undo last commit (licenses change) in this PR
create separate issue/PR specifically for the licenses update; make it a blocker for the release, if needed. Deal with licenses there.
add licenses validation into the PR validation

This is what's currently left for the server, after the last commit:

$ dev/check-binary-license bookkeeper-dist/server/target/bookkeeper-server-4.13.0-SNAPSHOT-bin.tar.gz 

com.google.android-annotations-4.1.1.4.jar unaccounted for in LICENSE
com.google.auto.value-auto-value-annotations-1.7.jar unaccounted for in LICENSE
com.google.code.gson-gson-2.8.6.jar unaccounted for in LICENSE
com.google.guava-failureaccess-1.0.1.jar unaccounted for in LICENSE
com.google.guava-guava-30.0-jre.jar unaccounted for in LICENSE
com.google.guava-listenablefuture-9999.0-empty-to-avoid-conflict-with-guava.jar unaccounted for in LICENSE
com.google.http-client-google-http-client-1.34.0.jar unaccounted for in LICENSE
com.google.http-client-google-http-client-jackson2-1.34.0.jar unaccounted for in LICENSE
com.google.j2objc-j2objc-annotations-1.3.jar unaccounted for in LICENSE
com.google.protobuf-protobuf-java-util-3.12.0.jar unaccounted for in LICENSE
com.google.re2j-re2j-1.2.jar unaccounted for in LICENSE
com.squareup.okhttp-okhttp-2.7.4.jar unaccounted for in LICENSE
io.dropwizard.metrics-metrics-core-3.1.0.jar unaccounted for in LICENSE
io.perfmark-perfmark-api-0.19.0.jar unaccounted for in LICENSE
org.apache.thrift-libthrift-0.12.0.jar unaccounted for in LICENSE
org.checkerframework-checker-qual-3.5.0.jar unaccounted for in LICENSE
org.conscrypt-conscrypt-openjdk-uber-2.5.1.jar unaccounted for in LICENSE
org.rocksdb-rocksdbjni-6.10.2.jar unaccounted for in LICENSE
org.xerial.snappy-snappy-java-1.1.7.jar unaccounted for in LICENSE
com.google.guava-guava-30.0.jar mentioned in LICENSE, but not bundled
org.rocksdb-rocksdbjni-5.13.1.jar mentioned in LICENSE, but not bundled
com.google.code.gson-gson-2.7.jar mentioned in LICENSE, but not bundled
com.squareup.okhttp-okhttp-2.5.0.jar mentioned in LICENSE, but not bundled
org.apache.thrift-libthrift-0.9.3.jar mentioned in LICENSE, but not bundled
org.rocksdb-rocksdbjni-5.13.1.jar mentioned in LICENSE, but not bundled
com.google.protobuf.nano-protobuf-javanano-3.0.0-alpha-5.jar mentioned in LICENSE, but not bundled
jline-jline-2.11.jar mentioned in LICENSE, but not bundled
org.checkerframework-checker-compat-qual-2.5.2.jar mentioned in LICENSE, but not bundled
io.netty-netty-tcnative-boringssl-static-2.0.20.Final.jar mentioned in NOTICE, but not bundled
io.grpc-grpc-protobuf-nano-1.33.0.jar mentioned in NOTICE, but not bundled
deps/protobuf-3.14.0/LICENSE linked from LICENSE, but not found in tarball
deps/javax.servlet-api-4.0.0/CDDL+GPL-1.1 linked from LICENSE, but not found in tarball
deps/protobuf-3.5.1/LICENSE bundled, but not linked from LICENSE
deps/javax.servlet-api-3.1.0/CDDL+GPL-1.1 bundled, but not linked from LICENSE

lhotari · 2021-02-09T07:58:36Z

    <dependency>
      <groupId>org.testcontainers</groupId>
      <artifactId>testcontainers</artifactId>
-      <scope>test</scope>


Why is <scope>test</scope> removed from testcontainers dependency?

resolved by 2d86dc7

lhotari

I placed one question about dependencies in metadata-drivers/etcd/pom.xml. The <scope>test</scope> was removed for testcontainers dependency. However it seems odd that the same file contains other test dependencies such as

    <dependency>
      <groupId>org.arquillian.cube</groupId>
      <artifactId>arquillian-cube-docker</artifactId>
      <version>${arquillian-cube.version}</version>
      <exclusions>
        <exclusion>
          <groupId>com.github.docker-java</groupId>
          <artifactId>*</artifactId>
        </exclusion>
      </exclusions>
    </dependency>
    <dependency>
      <groupId>org.jboss.arquillian.junit</groupId>
      <artifactId>arquillian-junit-standalone</artifactId>
      <version>${arquillian-junit.version}</version>
      <exclusions>
        <exclusion>
          <groupId>com.github.docker-java</groupId>
          <artifactId>*</artifactId>
        </exclusion>
      </exclusions>
    </dependency>

Is this intentional?

lhotari · 2021-02-09T08:08:37Z

+    <dependency>
+      <groupId>org.arquillian.cube</groupId>
+      <artifactId>arquillian-cube-docker</artifactId>
+      <version>${arquillian-cube.version}</version>


should have <scope>test</scope> ?

resolved by 2d86dc7

lhotari · 2021-02-09T08:08:55Z

+    <dependency>
+      <groupId>org.jboss.arquillian.junit</groupId>
+      <artifactId>arquillian-junit-standalone</artifactId>
+      <version>${arquillian-junit.version}</version>


should have <scope>test</scope> ?

resolved by 2d86dc7

eolivelli · 2021-02-09T08:12:31Z

@dlg99 I have added a commit in order to address @lhotari comment

lhotari

LGTM

eolivelli

LGTM

dlg99 mentioned this pull request Feb 3, 2021

Update grpc to 1.31.0 apache/pulsar#8403

Closed

nicoloboschi approved these changes Feb 4, 2021

View reviewed changes

eolivelli requested changes Feb 4, 2021

View reviewed changes

lhotari reviewed Feb 4, 2021

View reviewed changes

Comment thread pom.xml

lhotari requested changes Feb 4, 2021

View reviewed changes

dlg99 added 2 commits February 4, 2021 12:22

Upgrading GRPC version

385dfb4

removed unused imports

ca9074c

dlg99 added 3 commits February 4, 2021 17:34

grpc version update, some Cr feedback around use of BOM, etcd client …

d28113e

…update for metadata-drivers

WIP: updating to new etcd API; couple of files still need work

4b965a6

etcd update, take 2

e2809b0

dlg99 force-pushed the master-grpc-upgrade branch from 691ce7a to e2809b0 Compare February 5, 2021 23:06

dlg99 added 3 commits February 5, 2021 15:28

jetcd-test makes maven skip all tests

1449090

had to upgrade testcontainers (and deal with API changes) and downgra…

5fece6e

…de grpc a little (to 1.33) to get integration tests to work

rolling back netty

b3585b7

eolivelli reviewed Feb 8, 2021

View reviewed changes

First pass on licenses update

6d88531

lhotari reviewed Feb 9, 2021

View reviewed changes

set test scope

2d86dc7

lhotari approved these changes Feb 9, 2021

View reviewed changes

eolivelli approved these changes Feb 9, 2021

View reviewed changes

eolivelli changed the title ~~Upgrading GRPC version~~ Upgrading GRPC version, Netty to 4.1.50Final and ETCD client driver Feb 9, 2021

eolivelli changed the title ~~Upgrading GRPC version, Netty to 4.1.50Final and ETCD client driver~~ Upgrading GRPC version to 1.33, Netty to 4.1.50Final and ETCD client driver Feb 9, 2021

eolivelli added this to the 4.13.0 milestone Feb 9, 2021

eolivelli added area/build area/metadata area/tableservice changes related to table service area/tests release/4.13.0 type/improvement labels Feb 9, 2021

eolivelli assigned dlg99 Feb 9, 2021

eolivelli merged commit 27e7625 into apache:master Feb 9, 2021

dlg99 mentioned this pull request Feb 9, 2021

Licenses check finds multiple problems #2592

Closed

sijie mentioned this pull request Feb 9, 2021

ISSUE-2592: Licenses check finds multiple problems streamnative/bookkeeper-achieved#325

Closed

dlg99 deleted the master-grpc-upgrade branch October 14, 2021 23:42

Uh oh!

Conversation

dlg99 commented Feb 3, 2021 • edited by eolivelli Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Changes

Uh oh!

nicoloboschi left a comment

Choose a reason for hiding this comment

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dlg99 commented Feb 4, 2021

Uh oh!

dlg99 commented Feb 8, 2021

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

dlg99 commented Feb 9, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lhotari left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eolivelli commented Feb 9, 2021

Uh oh!

lhotari left a comment

Choose a reason for hiding this comment

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dlg99 commented Feb 3, 2021 •

edited by eolivelli

Loading