KAFKA-12758 Added `server-common` module to have server side common classes. #10638

satishd · 2021-05-06T06:23:42Z

Added server-common module to have server side common classes.
Moved ApiMessageAndVersion, RecordSerde, AbstractApiMessageSerde, and BytesApiMessageSerde to server-common module.

Existing tests are sufficient as this change is about moving files into a separate module.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

satishd · 2021-05-06T11:22:36Z

@junrao As discussed earlier, serde related classes are moved to the new server-common module.
We can add more server specific classes from other modules like clients into this module with followup PRs.

ijuma · 2021-05-06T13:36:57Z

cc @cmccabe

junrao

@satishd : Thanks for the PR. A few comments below.

junrao · 2021-05-06T15:54:42Z

build.gradle

@@ -1585,6 +1644,7 @@ project(':shell') {
    implementation libs.jacksonJDK8Datatypes
    implementation libs.jline
    implementation libs.slf4jApi
+    implementation project(':server-common')


Is this needed since core already depends on server-common?

Right, this is needed because core module does not expose server-common as a transitive module with gradle api dependency. It uses implementation dependency for server-common to be part of core module's compile/runtime classpaths.

I am not sure whether we really have usages for server-common classes to be exposed as part of core module.

core should generally not expose anything with api since core doesn't expose any public api.

junrao · 2021-05-06T16:14:15Z

build.gradle

@@ -779,6 +779,7 @@ project(':core') {
    api project(':clients')
    api libs.scalaLibrary

+    implementation project(':server-common')


Is that needed since server-common is included in metadata/storage already?

Right, it is needed as server-common is only compile/runtime dependency of raft and metadata. But it is not a transitive dependency.

junrao · 2021-05-06T16:15:25Z

build.gradle

@@ -1267,11 +1269,13 @@ project(':raft') {
  archivesBaseName = "kafka-raft"

  dependencies {
+    implementation project(':server-common')


Is this needed since server-common is included in metadata?

Pl see the earlier comment.

junrao · 2021-05-06T16:23:08Z

build.gradle

+  archivesBaseName = "kafka-server-common"
+
+  dependencies {
+    implementation project(':clients')


Since server-common depends on clients, perhaps we could remove clients from all modules that depend on server-common?

I did not add clients as api dependency. We may need to do that as the users of server-common need org.apache.kafka.common.protocol classes as they are part of the APIs in server-common classes.

After this change, we can remove clients dependency from wherever server-common module is directly used.

Added the change with the commit 1e259ff

I don't think we should remove the clients dependency since those modules rely on clients classes independently of their server-common dependency.

Good point @ijuma. It makes sense to me to have explicit client dependency irrespective of server-common dependency.

junrao · 2021-05-06T17:32:16Z

build.gradle

@@ -1070,6 +1071,7 @@ project(':metadata') {
  archivesBaseName = "kafka-metadata"

  dependencies {
+    implementation project(':server-common')


Not directly related to this PR. Do you know why we only include the storage module in releaseTarGz target, but not other server side modules?

Good catch. It looks like these are already copied as part of configurations.runtimeClasspath. I raised #10647 to fix that.

junrao · 2021-05-06T17:33:17Z

build.gradle

@@ -1345,6 +1349,62 @@ project(':raft') {
  }
 }

+project(':server-common') {
+  archivesBaseName = "kafka-server-common"


Do we want to move the classes in storage-api here? It helps reduce one module.

I plan to do that in another PR. I have not yet decided whether to have a separate sub module like server-api or part of server-common module. server-api module can contain all the server related public API classes including storage, security etc.

This is a good question and worth thinking about a bit more. I agree we can do it in a separate PR. I think the main thing to consider is how we differentiate between public classes exposed to users and common classes to be used by the kafka server modules.

Created https://issues.apache.org/jira/browse/KAFKA-12757 to discuss more on this.

Moved ApiMessageAndVersion, RecordSerde, AbstractApiMessageSerde, and BytesApiMessageSerde to server-common module.

…ntation' to 'api' to make it as a trasitive dependency as classes in 'server-common' module expose some of the 'clients' classes used. Addressed review comments

satishd · 2021-05-07T11:56:05Z

Thanks @junrao for the review comments. Pl see inline replies and the latest commits.

mumrah · 2021-05-11T15:46:37Z

build.gradle

+
+  sourceSets {
+    main {
+      java {


Are we only going to allow Java sources in this new module? (I think that's probably a good idea)

junrao

@satishd : Thanks for the updated PR. LGTM.

@kowshik : Any other comments from you?

kowshik

@satishd LGTM. Thanks for the PR!

…e-allocations-lz4 * apache-github/trunk: (155 commits) KAFKA-12728: Upgrade gradle to 7.0.2 and shadow to 7.0.0 (apache#10606) KAFKA-12778: Fix QuorumController request timeouts and electLeaders (apache#10688) KAFKA-12754: Improve endOffsets for TaskMetadata (apache#10634) Rework on KAFKA-3968: fsync the parent directory of a segment file when the file is created (apache#10680) MINOR: set replication.factor to 1 to make StreamsBrokerCompatibilityService work with old broker (apache#10673) MINOR: prevent cleanup() from being called while Streams is still shutting down (apache#10666) KAFKA-8326: Introduce List Serde (apache#6592) KAFKA-12697: Add Global Topic and Partition count metrics to the Quorum Controller (apache#10679) KAFKA-12648: MINOR - Add TopologyMetadata.Subtopology class for subtopology metadata (apache#10676) MINOR: Update jacoco to 0.8.7 for JDK 16 support (apache#10654) MINOR: exclude all `src/generated` and `src/generated-test` (apache#10671) KAFKA-12772: Move all transaction state transition rules into their states (apache#10667) KAFKA-12758 Added `server-common` module to have server side common classes. (apache#10638) MINOR Removed copying storage libraries specifically as they are already copied. (apache#10647) KAFKA-5876: KIP-216 Part 4, Apply InvalidStateStorePartitionException for Interactive Queries (apache#10657) KAFKA-12747: Fix flakiness in shouldReturnUUIDsWithStringPrefix (apache#10643) MINOR: remove unnecessary placeholder from WorkerSourceTask#recordSent (apache#10659) MINOR: Remove unused `scalatest` definition from `dependencies.gradle` (apache#10655) MINOR: checkstyle version upgrade: 8.20 -> 8.36.2 (apache#10656) KAFKA-12464: minor code cleanup and additional logging in constrained sticky assignment (apache#10645) ...

…lasses. (apache#10638) Added server-common module to have server side common classes. Moved ApiMessageAndVersion, RecordSerde, AbstractApiMessageSerde, and BytesApiMessageSerde to server-common module. Reivewers: Kowshik Prakasam <[email protected]>, Jun Rao <[email protected]>

…lasses. (apache#10638) Summary: Added server-common module to have server side common classes. Moved ApiMessageAndVersion, RecordSerde, AbstractApiMessageSerde, and BytesApiMessageSerde to server-common module. apache-reviewers: Kowshik Prakasam <[email protected]>, Jun Rao <[email protected]> (cherry picked from commit 7ef3879) Reviewers: #ldap_kafka_admins, kchandraprakash Reviewed By: #ldap_kafka_admins, kchandraprakash JIRA Issues: DKAFC-868 Differential Revision: https://code.uberinternal.com/D6303277

satishd force-pushed the server-common branch 2 times, most recently from ab75beb to 11bece6 Compare May 6, 2021 06:58

satishd changed the title ~~MINOR Added server-common module to have server side common classes.~~ KAFKA-12758 Added server-common module to have server side common classes. May 6, 2021

junrao reviewed May 6, 2021

View reviewed changes

satishd mentioned this pull request May 7, 2021

MINOR Removed copying storage libraries specifically as the task already copies them. #10647

Merged

3 tasks

satishd added 2 commits May 7, 2021 17:18

MINOR Added server-common module to have server side common classes.

2f30822

Moved ApiMessageAndVersion, RecordSerde, AbstractApiMessageSerde, and BytesApiMessageSerde to server-common module.

Changed 'clients' project dependency in 'server-common' from 'impleme…

1e259ff

…ntation' to 'api' to make it as a trasitive dependency as classes in 'server-common' module expose some of the 'clients' classes used. Addressed review comments

satishd force-pushed the server-common branch from 11bece6 to 1e259ff Compare May 7, 2021 11:53

Added explicit clients dependency as suggested in the comments.

ea1f7c6

mumrah reviewed May 11, 2021

View reviewed changes

junrao approved these changes May 11, 2021

View reviewed changes

kowshik approved these changes May 11, 2021

View reviewed changes

junrao merged commit 7ef3879 into apache:trunk May 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-12758 Added `server-common` module to have server side common classes. #10638

KAFKA-12758 Added `server-common` module to have server side common classes. #10638

satishd commented May 6, 2021 •

edited

Loading

satishd commented May 6, 2021

ijuma commented May 6, 2021

junrao left a comment

junrao May 6, 2021

satishd May 7, 2021

ijuma May 7, 2021

junrao May 6, 2021

satishd May 7, 2021

junrao May 6, 2021

satishd May 7, 2021

junrao May 6, 2021

satishd May 7, 2021 •

edited

Loading

satishd May 7, 2021

ijuma May 7, 2021

satishd May 7, 2021

junrao May 6, 2021

satishd May 7, 2021

junrao May 6, 2021

satishd May 7, 2021 •

edited

Loading

ijuma May 7, 2021

satishd May 10, 2021

satishd commented May 7, 2021 •

edited

Loading

mumrah May 11, 2021 •

edited

Loading

junrao left a comment

kowshik left a comment

KAFKA-12758 Added server-common module to have server side common classes. #10638

KAFKA-12758 Added server-common module to have server side common classes. #10638

Conversation

satishd commented May 6, 2021 • edited Loading

Committer Checklist (excluded from commit message)

satishd commented May 6, 2021

ijuma commented May 6, 2021

junrao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

satishd May 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

satishd May 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

satishd commented May 7, 2021 • edited Loading

mumrah May 11, 2021 • edited Loading

Choose a reason for hiding this comment

junrao left a comment

Choose a reason for hiding this comment

kowshik left a comment

Choose a reason for hiding this comment

KAFKA-12758 Added `server-common` module to have server side common classes. #10638

KAFKA-12758 Added `server-common` module to have server side common classes. #10638

satishd commented May 6, 2021 •

edited

Loading

satishd May 7, 2021 •

edited

Loading

satishd May 7, 2021 •

edited

Loading

satishd commented May 7, 2021 •

edited

Loading

mumrah May 11, 2021 •

edited

Loading