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.

Sign up for GitHub

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

Jump to bottom

KAFKA-15854: Move Java classes from kafka.server to the server module #14796

Merged
merged 3 commits into from
Nov 20, 2023
Merged

KAFKA-15854: Move Java classes from kafka.server to the server module #14796

merged 3 commits into from
Nov 20, 2023

Conversation

ijuma
Copy link
Contributor

@ijuma ijuma commented Nov 19, 2023 •
edited
Loading

We only move Java classes that have minimal or no dependencies on Scala classes in this PR.

Details:

  • Configured server module in build files.
  • Changed ControllerRequestCompletionHandler to be an interface since it has no implementations.
  • Cleaned up various import control files.
  • Minor build clean-ups for server-common.
  • Disabled testAssignmentAggregation when executed with Java 8, this is an existing issue (see MINOR: Fix unstable sorting in AssignmentsManagerTest #14794).

For broader context on this change, please check:

  • KAFKA-15852: Move server code from core to server module

Committer Checklist (excluded from commit message)

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

ijuma
ijuma commented Nov 19, 2023
View reviewed changes
build.gradle
srcDirs = ["src/test/java"]
}
}
}
Copy link
Contributor Author

@ijuma ijuma Nov 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Redundant.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we create a ticket to get rid of this from all sub-projects? If you like, we can do it as part of this PR itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can file a ticket. Some of them are actually necessary as they add additional directories.

ijuma
ijuma commented Nov 19, 2023
View reviewed changes
build.gradle
@@ -1670,6 1714,10 @@ project(':server-common') {
checkstyle {
configProperties = checkstyleConfigProperties("import-control-server-common.xml")
}

javadoc {
enabled = false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't have public classes in server-common, so make it explicit that javadoc should not be published.

ijuma
ijuma commented Nov 19, 2023
View reviewed changes
server/src/main/java/org/apache/kafka/server/metrics/ClientMetricsConfigs.java
@@ -14,7 14,7 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package kafka.metrics;
package org.apache.kafka.server.metrics;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@junrao @apoorvmittal10 Note that I have moved these recently introduced classes to this new module.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks @ijuma. @junrao I ll move the classes in other PRs to server/src package itself once review is complete. Moving prior active review will lose context of comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you please share the PRs where this is being discussed now?

@ijuma ijuma requested a review from showuon November 19, 2023 03:26
divijvaidya
divijvaidya reviewed Nov 19, 2023
View reviewed changes
Copy link
Contributor

@divijvaidya divijvaidya left a comment

Choose a reason for hiding this comment

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

Please add a package-info.java file in the server which can help new contributors to the code understand what components goes in the server module. At some stage we probably need a component level diagram of Kafka code base to improve onboarding to code base.

build.gradle Show resolved Hide resolved
build.gradle Show resolved Hide resolved
checkstyle/import-control-storage.xml
<allow pkg="java.security" />
<allow pkg="javax.net.ssl" />
<allow pkg="javax.security" />
<allow pkg="org.ietf.jgss" />
Copy link
Contributor

Choose a reason for hiding this comment

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

thank you for cleaning the import-control files!

build.gradle
srcDirs = ["src/test/java"]
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we create a ticket to get rid of this from all sub-projects? If you like, we can do it as part of this PR itself.

@ijuma ijuma force-pushed the kafka-15854-move-java-classes-to-server-module branch from 648a6c9 to f8c784a Compare November 19, 2023 16:46
@ijuma
Copy link
Contributor Author

ijuma commented Nov 19, 2023

@divijvaidya Thanks for the review. I added a basic package-info file. We can flesh it out as we add move code to this module.

@divijvaidya
Copy link
Contributor

Please feel free to merge after CI is successful.

@ijuma
Copy link
Contributor Author

ijuma commented Nov 20, 2023

I ran the tests a few times and the JDK 8 build passed, the failures for the other builds are unrelated:

Build / JDK 17 and Scala 2.13 / org.apache.kafka.trogdor.coordinator.CoordinatorTest.testTaskRequestWithOldStartMsGetsUpdated()
Build / JDK 21 and Scala 2.13 / org.apache.kafka.connect.integration.ConnectorRestartApiIntegrationTest.testMultiWorkerRestartOnlyConnector
Build / JDK 21 and Scala 2.13 / kafka.api.DelegationTokenEndToEndAuthorizationWithOwnerTest.testProduceConsumeViaAssign(String).quorum=kraft
Build / JDK 21 and Scala 2.13 / org.apache.kafka.controller.QuorumControllerMetricsIntegrationTest.testTimeoutMetrics()
Build / JDK 21 and Scala 2.13 / org.apache.kafka.tiered.storage.integration.TransactionsWithTieredStoreTest.testBumpTransactionalEpoch(String).quorum=kraft
Build / JDK 11 and Scala 2.13 / kafka.api.SaslPlainPlaintextConsumerTest.testCoordinatorFailover()

@ijuma ijuma merged commit df78204 into apache:trunk Nov 20, 2023
1 check failed
@ijuma ijuma deleted the kafka-15854-move-java-classes-to-server-module branch November 20, 2023 06:09
yyu1993 pushed a commit to yyu1993/kafka that referenced this pull request Feb 15, 2024
@ijuma @yyu1993
KAFKA-15854: Move Java classes from kafka.server to the server mo…
406384a 
…dule (apache#14796)

We only move Java classes that have minimal or no dependencies on Scala classes in this PR.

Details:
* Configured `server` module in build files.
* Changed `ControllerRequestCompletionHandler` to be an interface since it has no implementations.
* Cleaned up various import control files.
* Minor build clean-ups for `server-common`.
* Disabled `testAssignmentAggregation` when executed with Java 8, this is an existing issue (see apache#14794).

For broader context on this change, please check:
* KAFKA-15852: Move server code from `core` to `server` module

Reviewers: Divij Vaidya <[email protected]>
AnatolyPopov pushed a commit to aiven/kafka that referenced this pull request Feb 16, 2024
@ijuma @AnatolyPopov
KAFKA-15854: Move Java classes from kafka.server to the server mo…
ec5605e 
…dule (apache#14796)

We only move Java classes that have minimal or no dependencies on Scala classes in this PR.

Details:
* Configured `server` module in build files.
* Changed `ControllerRequestCompletionHandler` to be an interface since it has no implementations.
* Cleaned up various import control files.
* Minor build clean-ups for `server-common`.
* Disabled `testAssignmentAggregation` when executed with Java 8, this is an existing issue (see apache#14794).

For broader context on this change, please check:
* KAFKA-15852: Move server code from `core` to `server` module

Reviewers: Divij Vaidya <[email protected]>
@cwperks cwperks mentioned this pull request Mar 4, 2024
Merged
clolov pushed a commit to clolov/kafka that referenced this pull request Apr 5, 2024
@ijuma @clolov
KAFKA-15854: Move Java classes from kafka.server to the server mo…
635bec5 
…dule (apache#14796)

We only move Java classes that have minimal or no dependencies on Scala classes in this PR.

Details:
* Configured `server` module in build files.
* Changed `ControllerRequestCompletionHandler` to be an interface since it has no implementations.
* Cleaned up various import control files.
* Minor build clean-ups for `server-common`.
* Disabled `testAssignmentAggregation` when executed with Java 8, this is an existing issue (see apache#14794).

For broader context on this change, please check:
* KAFKA-15852: Move server code from `core` to `server` module

Reviewers: Divij Vaidya <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@apoorvmittal10 apoorvmittal10 apoorvmittal10 left review comments

@divijvaidya divijvaidya divijvaidya approved these changes

@showuon showuon Awaiting requested review from showuon

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ijuma @divijvaidya @apoorvmittal10