KAFKA-16225: Set metadata.log.dir to broker in KRAFT mode in integration test#15354
Conversation
|
@jolshan, @gharris1727, @gaurav-narula can you have a look please? |
| cfgs.foreach(_.setProperty(KafkaConfig.NewGroupCoordinatorEnableProp, "true")) | ||
| } | ||
|
|
||
| if(isKRaftTest()) { |
There was a problem hiding this comment.
nit: space after if
So the issue was that the metadata and other log directories were using the same temp directory?
There was a problem hiding this comment.
the issue was that we don't set metadata dir which in this case we fall back to the first log dir.
There was a problem hiding this comment.
To add, #15262 made it apparent because prior to it, the check for metadata log dir failure didn't handle relative paths correctly. Refer https://github.com/apache/kafka/pull/15262/files#diff-78812e247ffeae6f8c49b1b22506434701b1e1bafe7f92ef8f8708059e292bf0R2589
There was a problem hiding this comment.
The test was flaky because as we call causeLogDirFailure some times we impact the first log.dir which also is KafkaConfig.metadataLogDir as we don't have metadata.log.dir. So to fix the flakiness we need to explicitly set metadata.log.dir to diff log dir than the ones we could potentially fail for the tests. Hope this explains it!
|
I filed a JIRA for DescribeConsumerGroupTest https://issues.apache.org/jira/browse/KAFKA-16245 |
|
I still see one LogDirFailureTest failure: Can we confirm it is not because the log directory is missing? |
So far I can not reproduce this failure locally to debug it. But I think the test try to offline disk before the broker catchup with topic creation delta as topic creation is fire and forget. I pushed a comment that ensure that brokers catch-up first before start testing. |
|
What I noticed locally that |
OmniaGM
force-pushed
the
KAFKA-16225_fix_LogDirFailureTest_flakiness
branch
2 times, most recently
from
f38b268 to
a2bc46f
Compare
OmniaGM
force-pushed
the
KAFKA-16225_fix_LogDirFailureTest_flakiness
branch
from
a2bc46f to
5bf9b2f
Compare
|
Does this make sense to change across every test suite, not just LogDirFailureTest? |
If we want to limited to
I don't think it is a question of common practice or not (doc doesn't mention which is the recommended way to set this) but as no other tests try to fail metadata log dir or test where the metadata land it wouldn't matter. We don't have coverage anywhere as far as I can see for metadata log dir set separately or default! |
|
Ok -- I think in order to reduce the build failures we should merge this change. My understanding is that this doesn't introduce new failures. I also agree that it seems ok to make this change since the metadata log is special and it doesn't seem like log dir failure test was ever trying to test its failure modes. If we do want such testing we should also file a jira for that. |
|
I will keep the JIRA open as we try to figure out the fix of the other errors. |
| if (isKRaftTest()) { | ||
| val value = configs.map(c => c.brokerId -> c.logDirs.contains(c.metadataLogDir)) | ||
| logger.warn(s">>>>>> ${value.mkString(",")}") | ||
| } |
There was a problem hiding this comment.
Did you mean to include this change? The logging format and lack of description seems odd.
There was a problem hiding this comment.
I thought I removed this one!
There was a problem hiding this comment.
will raise a minor pr to remove it. I removed it but didn't push the commit ops
|
@jolshan Do you plan to backport this to 3.7? I recently try to backport other PRs to 3.7 and encounter the same flaky |
|
I have backport this PR to 3.7 6f76ed9 |
6 participants
Fix the flakiness of LogDirFailureTest by setting a separate metadata.log.dir for brokers in KRAFT mode
Committer Checklist (excluded from commit message)