Skip to content

MINOR: Always send cumulative failed dirs in HB request - #14770

Merged
cmccabe merged 1 commit into
apache:trunkfrom
soarez:m-hbfaildir
Nov 21, 2023
Merged

MINOR: Always send cumulative failed dirs in HB request#14770
cmccabe merged 1 commit into
apache:trunkfrom
soarez:m-hbfaildir

Conversation

@soarez

@soarez soarez commented Nov 15, 2023

Copy link
Copy Markdown
Member

Instead of only sending failed log directory UUIDs in the heartbeat request until a successful response is received, the broker sends the full cumulative set of failed directories since startup time.

This aims to simplify the handling of log directory failure in the controller side, considering overload mode handling of heartbeat requests, which returns an undifferentiated reply.

Committer Checklist (excluded from commit message)

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

Instead of only sending failed log directory UUIDs in the heartbeat
request until a successful response is received, the broker sends
the full cumulative set of failed directories since startup time.

This aims to simplify the handling of log directory failure in the
controller side, considering overload mode handling of heartbeat
requests, which returns an undifferentiated reply.
@soarez

soarez commented Nov 15, 2023

Copy link
Copy Markdown
Member Author

@pprovenzano @cmccabe

@pprovenzano pprovenzano 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

@cmccabe cmccabe 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. Thanks @soarez and @pprovenzano

@cmccabe
cmccabe merged commit c7c82ba into apache:trunk Nov 21, 2023

manager.propagateDirectoryFailure(Uuid.fromString("1iF76HVNRPqC7Y4r6647eg"))
poll(ctx, manager, heartbeats(4)).data()
val dirs3 = poll(ctx, manager, heartbeats(5)).data().offlineLogDirs()

@apoorvmittal10 apoorvmittal10 Nov 23, 2023

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.

@soarez Can we please verify the flakiness in the test. I have added @RepeatedTest(100) locally for this test and can always get failures among 100 runs. Listing out 2 failures below

java.util.ConcurrentModificationException
	at java.base/java.util.ArrayDeque.nonNullElementAt(ArrayDeque.java:270)
	at java.base/java.util.ArrayDeque$DeqIterator.next(ArrayDeque.java:700)
	at kafka.server.MockNodeToControllerChannelManager.poll(MockNodeToControllerChannelManager.scala:73)
	at kafka.server.RegistrationTestContext.poll(RegistrationTestContext.scala:76)
	at kafka.server.BrokerLifecycleManagerTest.poll(BrokerLifecycleManagerTest.scala:202)
	at kafka.server.BrokerLifecycleManagerTest.testAlwaysSendsAccumulatedOfflineDirs(BrokerLifecycleManagerTest.scala:230)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.accept(ForEachOps.java:183)
	at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:197)
	at java.base/java.util.stream.ReferencePipeline$2$1.accept(ReferencePipeline.java:179)
	at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:197)
	at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.accept(ForEachOps.java:183)
	at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:197)
	at java.base/java.util.stream.IntPipeline$1$1.accept(IntPipeline.java:180)
	at java.base/java.util.stream.Streams$RangeIntSpliterator.forEachRemaining(Streams.java:104)
	at java.base/java.util.Spliterator$OfInt.forEachRemaining(Spliterator.java:711)
	at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:509)
	at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:499)
	at java.base/java.util.stream.ForEachOps$ForEachOp.evaluateSequential(ForEachOps.java:150)
	at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.evaluateSequential(ForEachOps.java:173)
	at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
	at java.base/java.util.stream.ReferencePipeline.forEach(ReferencePipeline.java:596)
	at java.base/java.util.stream.ReferencePipeline$7$1.accept(ReferencePipeline.java:276)
rg.opentest4j.AssertionFailedError: 
Expected :Set(h3sC4Yk-Q9-fd0ntJTocCA, ej8Q9_d2Ri6FXNiTxKFiow, 1iF76HVNRPqC7Y4r6647eg)
Actual   :Set(h3sC4Yk-Q9-fd0ntJTocCA, ej8Q9_d2Ri6FXNiTxKFiow)
<Click to see difference>


	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
	at org.junit.jupiter.api.AssertEquals.failNotEqual(AssertEquals.java:197)
	at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:182)
	at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:177)
	at org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:1141)
	at kafka.server.BrokerLifecycleManagerTest.testAlwaysSendsAccumulatedOfflineDirs(BrokerLifecycleManagerTest.scala:238)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.accept(ForEachOps.java:183)
	at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:197)
	at java.base/java.util.stream.ReferencePipeline$2$1.accept(ReferencePipeline.java:179)
	at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:197)
	at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.accept(ForEachOps.java:183)
	at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:197)
	at java.base/java.util.stream.IntPipeline$1$1.accept(IntPipeline.java:180)
	at java.base/java.util.stream.Streams$RangeIntSpliterator.forEachRemaining(Streams.java:104)

cc: @cmccabe @junrao (Saw the flakiness in the run at PR build: #14699, https://ci-builds.apache.org/blue/organizations/jenkins/Kafka%2Fkafka-pr/detail/PR-14699/17/tests/)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks for reporting this Apoorv. Please see #14836

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 fixing this @soarez.

yyu1993 pushed a commit to yyu1993/kafka that referenced this pull request Feb 15, 2024
Instead of only sending failed log directory UUIDs in the heartbeat
request until a successful response is received, the broker sends
the full cumulative set of failed directories since startup time.

This aims to simplify the handling of log directory failure in the
controller side, considering overload mode handling of heartbeat
requests, which returns an undifferentiated reply.

Reviewers: Colin P. McCabe <cmccabe@apache.org>, Proven Provenzano <pprovenzano@confluent.io>
AnatolyPopov pushed a commit to aiven/kafka that referenced this pull request Feb 16, 2024
Instead of only sending failed log directory UUIDs in the heartbeat
request until a successful response is received, the broker sends
the full cumulative set of failed directories since startup time.

This aims to simplify the handling of log directory failure in the
controller side, considering overload mode handling of heartbeat
requests, which returns an undifferentiated reply.

Reviewers: Colin P. McCabe <cmccabe@apache.org>, Proven Provenzano <pprovenzano@confluent.io>
clolov pushed a commit to clolov/kafka that referenced this pull request Apr 5, 2024
Instead of only sending failed log directory UUIDs in the heartbeat
request until a successful response is received, the broker sends
the full cumulative set of failed directories since startup time.

This aims to simplify the handling of log directory failure in the
controller side, considering overload mode handling of heartbeat
requests, which returns an undifferentiated reply.

Reviewers: Colin P. McCabe <cmccabe@apache.org>, Proven Provenzano <pprovenzano@confluent.io>
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