Skip to content

KAFKA-12258: Add support for splitting appending records#10063

Merged
hachikuji merged 9 commits into
apache:trunkfrom
jsancio:kafka-12258-split-batches
Feb 19, 2021
Merged

KAFKA-12258: Add support for splitting appending records#10063
hachikuji merged 9 commits into
apache:trunkfrom
jsancio:kafka-12258-split-batches

Conversation

@jsancio

@jsancio jsancio commented Feb 5, 2021

Copy link
Copy Markdown
Member
  1. Type BatchAccumulator. Add support for appending records into one or more batches.
  2. Type RaftClient. Rename scheduleAppend to scheduleAtomicAppend.
  3. Type RaftClient. Add a new method scheduleAppend which appends records to the log using as many batches as necessary.
  4. Increase the batch size from 1MB to 8MB.

More detailed description of your change,
if necessary. The PR title and PR message become
the squashed commit message, so use a separate
comment to ping reviewers.

Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.

Committer Checklist (excluded from commit message)

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

@cmccabe cmccabe added the kraft label Feb 5, 2021
Comment thread raft/src/main/java/org/apache/kafka/raft/internals/BatchAccumulator.java Outdated
@jsancio

jsancio commented Feb 11, 2021

Copy link
Copy Markdown
Member Author

@mumrah I found a few more issues on how we compute the needed sizes. I need to fix one of the remaining issues and write tests for them. Let's assume that this PR is still a WIP.

@jsancio

jsancio commented Feb 15, 2021

Copy link
Copy Markdown
Member Author

@mumrah Ready for review. Fixed the size calculation issues that I discovered when implementing this PR.

@hachikuji hachikuji left a comment

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.

Thanks for the patch. I think we might need changes in RaftManager as well to ensure that max.message.bytes gets set correctly for the @metadata log. I think currently we take the configuration from the general kafka configuration, which is probably not what we want. I wonder if there are any other overridable configurations that could cause us trouble.

Comment thread raft/src/main/java/org/apache/kafka/raft/RaftClient.java Outdated
Comment thread raft/src/main/java/org/apache/kafka/raft/internals/BatchAccumulator.java Outdated
Comment thread raft/src/main/java/org/apache/kafka/raft/internals/BatchAccumulator.java Outdated
Comment thread raft/src/main/java/org/apache/kafka/raft/internals/BatchAccumulator.java Outdated
Comment thread raft/src/main/java/org/apache/kafka/raft/internals/BatchBuilder.java Outdated
Comment thread raft/src/main/java/org/apache/kafka/raft/internals/BatchAccumulator.java Outdated
Comment thread raft/src/test/java/org/apache/kafka/raft/internals/BatchAccumulatorTest.java Outdated

@hachikuji hachikuji left a comment

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

@jsancio

jsancio commented Feb 19, 2021

Copy link
Copy Markdown
Member Author

Running the following commands

./gradlew -version

------------------------------------------------------------
Gradle 6.8.1
------------------------------------------------------------

Build time:   2021-01-22 13:20:08 UTC
Revision:     31f14a87d93945024ab7a78de84102a3400fa5b2

Kotlin:       1.4.20
Groovy:       2.5.12
Ant:          Apache Ant(TM) version 1.10.9 compiled on September 27 2020
JVM:          1.8.0_282 (Private Build 25.282-b08)
OS:           Linux 5.8.0-7642-generic amd64

./gradlew -PscalaVersion=2.12 clean compileJava compileScala compileTestJava compileTestScala spotlessScalaCheck checkstyleMain checkstyleTest spotbugsMain rat --profile --no-daemon --continue -PxmlSpotBugsReport=true

./gradlew -PscalaVersion=2.13 clean compileJava compileScala compileTestJava compileTestScala spotlessScalaCheck checkstyleMain checkstyleTest spotbugsMain rat --profile --no-daemon --continue -PxmlSpotBugsReport=true

./gradlew -PscalaVersion=2.12 unitTest integrationTest --profile --no-daemon --continue -PtestLoggingEvents=started,passed,skipped,failed -PignoreFailures=true -PmaxTestRetries=1 -PmaxTestRetryFailures=5

@jsancio

jsancio commented Feb 19, 2021

Copy link
Copy Markdown
Member Author

The following tests failed:

core:

    ConfigCommandTest. shouldFailIfUnresolvableHost()
    ConfigCommandTest. shouldFailIfUnresolvableHost()
    ConfigCommandTest. shouldFailIfUnresolvableHostUsingZookeeper()
    ConfigCommandTest. shouldFailIfUnresolvableHostUsingZookeeper()
    DynamicConfigChangeTest. testIpHandlerUnresolvableAddress()
    DynamicConfigChangeTest. testIpHandlerUnresolvableAddress()
    DynamicConfigTest. shouldFailIpConfigsWithBadHost()
    DynamicConfigTest. shouldFailIpConfigsWithBadHost()

@hachikuji
hachikuji merged commit 9243c10 into apache:trunk Feb 19, 2021
@jsancio
jsancio deleted the kafka-12258-split-batches branch February 19, 2021 04:46
hachikuji pushed a commit that referenced this pull request Feb 19, 2021
1. Type `BatchAccumulator`. Add support for appending records into one or more batches.
2. Type `RaftClient`. Rename `scheduleAppend` to `scheduleAtomicAppend`.
3. Type `RaftClient`. Add a new method `scheduleAppend` which appends records to the log using as many batches as necessary.
4. Increase the batch size from 1MB to 8MB.

Reviewers: David Arthur <mumrah@gmail.com>, Jason Gustafson <jason@confluent.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants