You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2021/02/17 05:27:56 UTC
[GitHub] [kafka] jsancio opened a new pull request #10138: KAFKA-12331: Use LEO for the base offset of LeaderChangeMessage batch
jsancio opened a new pull request #10138:
URL: https://github.com/apache/kafka/pull/10138
WIP
*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)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jsancio commented on pull request #10138: KAFKA-12331: Use LEO for the base offset of LeaderChangeMessage batch
Posted by GitBox <gi...@apache.org>.
jsancio commented on pull request #10138:
URL: https://github.com/apache/kafka/pull/10138#issuecomment-781730638
Ran the following commands locally:
```
$ ./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 -PmaxParallelForks=2 -PmaxTestRetries=1 -PmaxTestRetryFailures=5
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jsancio commented on pull request #10138: KAFKA-12331: Use LEO for the base offset of LeaderChangeMessage batch
Posted by GitBox <gi...@apache.org>.
jsancio commented on pull request #10138:
URL: https://github.com/apache/kafka/pull/10138#issuecomment-780816612
@hachikuji this PR is ready for review. Thanks!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] hachikuji commented on a change in pull request #10138: KAFKA-12331: Use LEO for the base offset of LeaderChangeMessage batch
Posted by GitBox <gi...@apache.org>.
hachikuji commented on a change in pull request #10138:
URL: https://github.com/apache/kafka/pull/10138#discussion_r577914843
##########
File path: raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientSnapshotTest.java
##########
@@ -106,7 +106,8 @@ public void testFetchRequestWithLargerLastFetchedEpoch() throws Exception {
OffsetAndEpoch oldestSnapshotId = new OffsetAndEpoch(3, 2);
RaftClientTestContext context = new RaftClientTestContext.Builder(localId, voters)
- .appendToLog(oldestSnapshotId.offset, oldestSnapshotId.epoch, Arrays.asList("a", "b", "c"))
+ .appendToLog(oldestSnapshotId.epoch, Arrays.asList("a", "b", "c"))
+ .appendToLog(oldestSnapshotId.epoch, Arrays.asList("a", "b", "c"))
Review comment:
nit: could we use a different records from the first append in these tests? It make them a little easier to follow
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jsancio commented on pull request #10138: KAFKA-12331: Use LEO for the base offset of LeaderChangeMessage batch
Posted by GitBox <gi...@apache.org>.
jsancio commented on pull request #10138:
URL: https://github.com/apache/kafka/pull/10138#issuecomment-781734293
We got the following failures:
```
core:
ConfigCommandTest. shouldFailIfUnresolvableHost()
ConfigCommandTest. shouldFailIfUnresolvableHostUsingZookeeper()
DelegationTokenCommandTest. testDelegationTokenRequests()
MetricsTest. testMetrics()
DynamicConfigChangeTest. testIpHandlerUnresolvableAddress()
DynamicConfigTest. shouldFailIpConfigsWithBadHost()
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jsancio commented on a change in pull request #10138: KAFKA-12331: Use LEO for the base offset of LeaderChangeMessage batch
Posted by GitBox <gi...@apache.org>.
jsancio commented on a change in pull request #10138:
URL: https://github.com/apache/kafka/pull/10138#discussion_r577868426
##########
File path: raft/src/test/java/org/apache/kafka/raft/MockLog.java
##########
@@ -250,30 +249,7 @@ private LogEntry buildEntry(Long offset, SimpleRecord record) {
@Override
public LogAppendInfo appendAsLeader(Records records, int epoch) {
- if (records.sizeInBytes() == 0)
- throw new IllegalArgumentException("Attempt to append an empty record set");
-
- long baseOffset = endOffset().offset;
- AtomicLong offsetSupplier = new AtomicLong(baseOffset);
- for (RecordBatch batch : records.batches()) {
- List<LogEntry> entries = buildEntries(batch, record -> offsetSupplier.getAndIncrement());
- appendBatch(new LogBatch(epoch, batch.isControlBatch(), entries));
- }
-
- return new LogAppendInfo(baseOffset, offsetSupplier.get() - 1);
- }
-
- LogAppendInfo appendAsLeader(Collection<SimpleRecord> records, int epoch) {
Review comment:
This method was only used for tests. To simplify the implementation, I removed this method and added a helper method to `MockLogTest`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jsancio commented on a change in pull request #10138: KAFKA-12331: Use LEO for the base offset of LeaderChangeMessage batch
Posted by GitBox <gi...@apache.org>.
jsancio commented on a change in pull request #10138:
URL: https://github.com/apache/kafka/pull/10138#discussion_r577868426
##########
File path: raft/src/test/java/org/apache/kafka/raft/MockLog.java
##########
@@ -250,30 +249,7 @@ private LogEntry buildEntry(Long offset, SimpleRecord record) {
@Override
public LogAppendInfo appendAsLeader(Records records, int epoch) {
- if (records.sizeInBytes() == 0)
- throw new IllegalArgumentException("Attempt to append an empty record set");
-
- long baseOffset = endOffset().offset;
- AtomicLong offsetSupplier = new AtomicLong(baseOffset);
- for (RecordBatch batch : records.batches()) {
- List<LogEntry> entries = buildEntries(batch, record -> offsetSupplier.getAndIncrement());
- appendBatch(new LogBatch(epoch, batch.isControlBatch(), entries));
- }
-
- return new LogAppendInfo(baseOffset, offsetSupplier.get() - 1);
- }
-
- LogAppendInfo appendAsLeader(Collection<SimpleRecord> records, int epoch) {
Review comment:
This method was only used for tests. To simplify the implementation, I remove this method and added a helper method to `MockLogTest`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jsancio commented on a change in pull request #10138: KAFKA-12331: Use LEO for the base offset of LeaderChangeMessage batch
Posted by GitBox <gi...@apache.org>.
jsancio commented on a change in pull request #10138:
URL: https://github.com/apache/kafka/pull/10138#discussion_r577972986
##########
File path: raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientSnapshotTest.java
##########
@@ -106,7 +106,8 @@ public void testFetchRequestWithLargerLastFetchedEpoch() throws Exception {
OffsetAndEpoch oldestSnapshotId = new OffsetAndEpoch(3, 2);
RaftClientTestContext context = new RaftClientTestContext.Builder(localId, voters)
- .appendToLog(oldestSnapshotId.offset, oldestSnapshotId.epoch, Arrays.asList("a", "b", "c"))
+ .appendToLog(oldestSnapshotId.epoch, Arrays.asList("a", "b", "c"))
+ .appendToLog(oldestSnapshotId.epoch, Arrays.asList("a", "b", "c"))
Review comment:
Yeah. The tests are getting hard to follow and write. When we have finish some of the more important tasks, I want to revisit `RaftClientTestContext` API.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] hachikuji merged pull request #10138: KAFKA-12331: Use LEO for the base offset of LeaderChangeMessage batch
Posted by GitBox <gi...@apache.org>.
hachikuji merged pull request #10138:
URL: https://github.com/apache/kafka/pull/10138
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org