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

Issue 8558: Upgrade Kubernetes client and remove sundr-codegen (allow Pulsar to run on JDK14+) #8576

Merged

Conversation

eolivelli
Copy link
Contributor

@eolivelli eolivelli commented Nov 16, 2020

Fixes #8558

Motivation

sundr-codegen jar is not needed at runtime and in bundles legacy versions of commons-lang and other libraries, this is polluting the runtime classpath of Pulsar Broker,Bookie,Functions...
The depedency comes from Kubernetes java client, used by Pulsar Functions.

sundrio/sundrio#153

Modifications

  • remove sundr-codegen from distribution lib
  • allow Pulsar bookie to run on JDK14+
  • upgrade Kubernetes java client
  • upgrade commons-lang3

Verifying this change

I have verified this change manually.

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

- remove sundr-codegen from distribution lib
- allow Pulsar bookie to run on JDK14+
@eolivelli
Copy link
Contributor Author

eolivelli commented Nov 16, 2020

@codelipenghui PTAL to this change as well, otherwise it is not possible to run Pulsar on JDK14+ using the standard package.

at MagNews.com we are already running Pulsar Broker on JDK14 and also JDK15 without issues, but we are not using the ASF package

@sijie
Copy link
Member

sijie commented Nov 17, 2020

@eolivelli Have you verified if this change works for running functions with Kubernetes runtime?

@eolivelli
Copy link
Contributor Author

@sijie no I didn't and I do not have a way to do it right now, I am sorry.

It would be really better to test it

@sijie
Copy link
Member

sijie commented Nov 18, 2020

@eolivelli I would like to hold on this until 2.7.0 is released. Because we found upgrading Kubernetes version potentially broke the build in the past (see #8601).

@eolivelli
Copy link
Contributor Author

@sijie
not a problem from my side.
I think it is safer to postpone

Copy link
Member

@sijie sijie left a comment

Choose a reason for hiding this comment

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

I think we should add an integration test framework to test kubernetes runtime. @wolfstudy Can you help with that?

@eolivelli
Copy link
Contributor Author

@wolfstudy feel free to pick up this patch
thank you

@eolivelli
Copy link
Contributor Author

@wolfstudy do you have time to pick up this issue ?
it is very annoying for people running on a recent Java version

@wolfstudy
Copy link
Member

@eolivelli Sorry, I missed this message, Sure, I will try to add an integration test to cover this change

@eolivelli
Copy link
Contributor Author

eolivelli commented Jan 18, 2021

@wolfstudy if you do not have cycles
Can you please point me to some other kind of k8s tests we have here in Pulsar ?
This way I can try to add the necessary tests

@eolivelli
Copy link
Contributor Author

@wolfstudy @sijie I am going to try out this brach on a K8s cluster manually.
Any suggestion about where to put an integration tests is very appreciated.

@eolivelli
Copy link
Contributor Author

@sijie @wolfstudy
I have tested locally this branch (using the helm-chart) and it works very well.
I believe there is no need to add integration tests in order to commit this patch.

We can add system tests, probably the best place would be nearby the helm-chart and not in this repository.

@eolivelli
Copy link
Contributor Author

/pulsarbot run-failure-checks

1 similar comment
@eolivelli
Copy link
Contributor Author

/pulsarbot run-failure-checks

@sijie
Copy link
Member

sijie commented Feb 23, 2021

@eolivelli I think the integration tests should be added in this repo. The helm chart repo is used for testing the helm chart. You can clone the helm chart repo in the CI job to start a KIND cluster for testing.

@eolivelli
Copy link
Contributor Author

@eolivelli I think the integration tests should be added in this repo. The helm chart repo is used for testing the helm chart. You can clone the helm chart repo in the CI job to start a KIND cluster for testing.

@sijie agreed, we can make it here (or in the new 'release repo' once PIP-62 is completed
but I would keep it out of the scope of this patch

I created a new issue
#9687

@eolivelli
Copy link
Contributor Author

@rdhabalia @merlimat @codelipenghui PTAL as well

@eolivelli eolivelli changed the title Issue 8558: Upgrade Kubernetes client and remove sundr-codegen Issue 8558: Upgrade Kubernetes client and remove sundr-codegen (allow Pulsar to run on JDK14+) Feb 26, 2021
@eolivelli
Copy link
Contributor Author

@codelipenghui @congbobo184 @wolfstudy can you please take a look ?

@eolivelli
Copy link
Contributor Author

I have merged with current master in order to bootstrap CI again.

Thank you @sijie for taking a look

@eolivelli
Copy link
Contributor Author

/pulsarbot run-failure-tests

@eolivelli
Copy link
Contributor Author

@wolfstudy can you please help merging this patch ?

@eolivelli
Copy link
Contributor Author

/pulsarbot run-failure-tests

@eolivelli
Copy link
Contributor Author

eolivelli commented Mar 9, 2021

/pulsarbot rerun-failure-checks

1 similar comment
@eolivelli
Copy link
Contributor Author

/pulsarbot rerun-failure-checks

@eolivelli
Copy link
Contributor Author

@wolfstudy @codelipenghui @rdhabalia CI finally passed.
I believe this is a good time to merge this patch

@codelipenghui codelipenghui merged commit 1d6aa57 into apache:master Mar 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot run Pulsar Bookie on JDK14+
4 participants