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

[FLINK-32031] Flink GCP Connector having issues with Conscrypt library #13

Closed
wants to merge 0 commits into from
Closed

Conversation

jayadeep-jayaraman
Copy link
Contributor

Upgraded the gcp bom library and added the relevant libraries to the project which have been removed from the underlying bom library

Updated one of the API's of pubsub as the original one is no longer used

@boring-cyborg
Copy link

boring-cyborg bot commented Jun 25, 2023

Thanks for opening this pull request! Please check out our contributing guidelines. (https://flink.apache.org/contributing/how-to-contribute.html)

@jayadeep-jayaraman jayadeep-jayaraman changed the title [FLINK-32031] Flink GCP Connector having issues with Conscrypt library FLINK-32031 Flink GCP Connector having issues with Conscrypt library Jun 25, 2023
@jayadeep-jayaraman
Copy link
Contributor Author

@MartijnVisser - can you please take a look at this change?

@jayadeep-jayaraman jayadeep-jayaraman changed the title FLINK-32031 Flink GCP Connector having issues with Conscrypt library [FLINK-32031] Flink GCP Connector having issues with Conscrypt library Jun 26, 2023
@MartijnVisser
Copy link
Contributor

@jayadeep-jayaraman Thanks for the PR! Just for my understanding, is Conscrypt a requirement when using newer Dataproc clusters?

Copy link
Contributor

@MartijnVisser MartijnVisser left a comment

Choose a reason for hiding this comment

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

@jayadeep-jayaraman I think this is looking fine overall. Can you restore the deleted .idea/vcs.xml file?

Copy link
Contributor

@MartijnVisser MartijnVisser left a comment

Choose a reason for hiding this comment

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

@jayadeep-jayaraman I realized that CI wasn't running properly on PRs, so that's now fixed and now I've re-ran it on your PR. However, there are some dependency convergence issues causing the CI to fail. Can you take a look at those?

@jayadeep-jayaraman
Copy link
Contributor Author

Hi @MartijnVisser - I have added back the vcs.xml file and yes the conscrypt issue is happening with newer versions of Dataproc where the dataproc image provides the latest conscrypt jar whereas the connector via the google-cloud-libraries-bom was packing an older version of conscrypt library.

Regarding the dependency convergence issue I checked and found that these are happening because the google-cloud-libraries-bom version was upgraded. Can you suggest how to resolve these issues ?

@MartijnVisser
Copy link
Contributor

Regarding the dependency convergence issue I checked and found that these are happening because the google-cloud-libraries-bom version was upgraded. Can you suggest how to resolve these issues ?

We usually pin the version of the dependency that has converged. See https://github.com/apache/flink-connector-elasticsearch/blob/main/pom.xml#L312-L317 for an example

Copy link
Contributor

@MartijnVisser MartijnVisser left a comment

Choose a reason for hiding this comment

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

LGTM. @jayadeep-jayaraman Do you also want to create a backport of this commit to branch v3.0, so we can release this in the next GCP PubSub release?

Copy link
Contributor

@MartijnVisser MartijnVisser left a comment

Choose a reason for hiding this comment

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

@jayadeep-jayaraman I actually can't merge this PR, because it contains a merge commit. Normally I would rebase myself and push the changes, but since you've built this on top of main in your fork, I can't push to that branch. So you'll need to rebase yourself and force push these changes

@jayadeep-jayaraman
Copy link
Contributor Author

@jayadeep-jayaraman I actually can't merge this PR, because it contains a merge commit. Normally I would rebase myself and push the changes, but since you've built this on top of main in your fork, I can't push to that branch. So you'll need to rebase yourself and force push these changes

are you referring to pushing the changing to 3.0 branch? Will the changes be merged to both the main and 3.0 branch? If yes then can this change be merged with main and I will create a separate PR for 3.0 branch with my changes, will that work?

@MartijnVisser
Copy link
Contributor

are you referring to pushing the changing to 3.0 branch?

No, also in order to merge this to main, all commits in this PR need to be rebased and preferably also squashed. I can normally do that as a committer, but since your forked main branch is also protected, I can't force push to that branch.

Will the changes be merged to both the main and 3.0 branch?

Yes, because v3.0 is the currently actively released version and there hasn't been a reason yet for another new, minor version of this connector. So we can backport it for the next patch release of v3.0, but we need to make sure that this is also included for the next time a new minor/major version needs to be created.

@jayadeep-jayaraman
Copy link
Contributor Author

Made the changes, pls check

@MartijnVisser
Copy link
Contributor

@jayadeep-jayaraman I'm a bit surprised that d7ed02b is now appearing in this PR as a commit, that shouldn't have been the case when you rebased 😅

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.

2 participants