Skip to content

chore: add rocksdbjni as explicit dependency to config setter module#7262

Merged
Victoria Xia (vcrfxia) merged 1 commit into
confluentinc:masterfrom
vcrfxia:rocksdbjni-explicit
Mar 17, 2021
Merged

chore: add rocksdbjni as explicit dependency to config setter module#7262
Victoria Xia (vcrfxia) merged 1 commit into
confluentinc:masterfrom
vcrfxia:rocksdbjni-explicit

Conversation

@vcrfxia

@vcrfxia Victoria Xia (vcrfxia) commented Mar 17, 2021

Copy link
Copy Markdown
Contributor

Description

The master build is currently failing because the ksqldb-rocksdb-config-setter module can't find dependencies from org.rocksdb. This dependency was previously being pulled in transitively from the module's kafka-streams dependency but this no longer appears to be the case. (The transitive dependency is now pulled in with scope runtime which I believe explains the compilation error.) This PR fixes the build by adding the dependency explicitly.

Weirdly, I can't find any recent changes to explain the change in behavior; if anyone else has more ideas for where to look that'd be great. The build started failing between 10:18am and 11:51am PST today.

Update: mystery has been solved (thanks, Sophie!). Here's the culprit (a change in Streams's transitive dependency handling): confluentinc/kafka@7a3ebbe#diff-49a96e7eea8a94af862798a45174e6ac43eb4f8b4bd40759b5da63ba31ec3ef7R1428

Testing done

Build passes.

Reviewer checklist

  • Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
  • Ensure relevant issues are linked (description should include text like "Fixes #")

@vcrfxia
Victoria Xia (vcrfxia) requested a review from a team as a code owner March 17, 2021 21:33

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🤷‍♂️ LGTM

<dependency>
<groupId>org.rocksdb</groupId>
<artifactId>rocksdbjni</artifactId>
<version>5.18.4</version>

@stevenpyzhang Steven Zhang (stevenpyzhang) Mar 17, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: we could put this version number in the properties section in the ksql parent pom

@vcrfxia Victoria Xia (vcrfxia) Mar 17, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Normally I'd agree but I don't feel strongly in this case since ksqlDB shouldn't be in the business of interacting with RocksDB directly (we should be going through Streams APIs for that) which means this dependency is specific to this config setter module.

OTOH, it's very little effort to move it and maybe we'll add more special modules later. I'll move it. Looks like we're going to try to get the transitive dependency put back in Streams so users don't have to keep their rocksdb versions in sync, which means this PR will likely get backed out so I'm going to leave it as is rather than restarting the build with a new commit.

@ableegoldman

Copy link
Copy Markdown
Contributor

I think it was probably this which was just synced over from AK earlier today.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm pretty concerned that we would need to pull in this dependency manually from now on, so we might need to revert this change in AK. But that will probably take a while to sync over so we should go ahead and merge this to unblock the build for now -- thanks for the PR

@ableegoldman

Copy link
Copy Markdown
Contributor

Update: we can revert this once apache/kafka#10341 is merged and has been synced over to ccs-kafka

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants