You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "brandboat (via GitHub)" <gi...@apache.org> on 2024/06/04 07:34:29 UTC
[PR] KAFKA-16888 Fix failed StorageToolTest.testFormatSucceedsIfAllDirectoriesAreAvailable and StorageToolTest.testFormatEmptyDirectory [kafka]
brandboat opened a new pull request, #16186:
URL: https://github.com/apache/kafka/pull/16186
a quick fix for https://issues.apache.org/jira/browse/KAFKA-16888
### 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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16888 Fix failed StorageToolTest.testFormatSucceedsIfAllDirectoriesAreAvailable and StorageToolTest.testFormatEmptyDirectory [kafka]
Posted by "brandboat (via GitHub)" <gi...@apache.org>.
brandboat commented on code in PR #16186:
URL: https://github.com/apache/kafka/pull/16186#discussion_r1625550301
##########
core/src/test/scala/unit/kafka/tools/StorageToolTest.scala:
##########
@@ -213,7 +213,7 @@ Found problem:
val availableDirs = Seq(TestUtils.tempDir(), TestUtils.tempDir(), TestUtils.tempDir()).map(dir => dir.toString)
val stream = new ByteArrayOutputStream()
assertEquals(0, runFormatCommand(stream, availableDirs))
- val actual = stream.toString().split("\\r?\\n")
+ val actual = stream.toString().split("\\r?\\n").drop(1)
val expect = availableDirs.map("Formatting %s".format(_))
assertEquals(availableDirs.size, actual.size)
Review Comment:
I've addressed comments in the latest commit, thanks for reviewing.
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16888 Fix failed StorageToolTest.testFormatSucceedsIfAllDirectoriesAreAvailable and StorageToolTest.testFormatEmptyDirectory [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 merged PR #16186:
URL: https://github.com/apache/kafka/pull/16186
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16888 Fix failed StorageToolTest.testFormatSucceedsIfAllDirectoriesAreAvailable and StorageToolTest.testFormatEmptyDirectory [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #16186:
URL: https://github.com/apache/kafka/pull/16186#discussion_r1625512865
##########
core/src/test/scala/unit/kafka/tools/StorageToolTest.scala:
##########
@@ -213,7 +213,7 @@ Found problem:
val availableDirs = Seq(TestUtils.tempDir(), TestUtils.tempDir(), TestUtils.tempDir()).map(dir => dir.toString)
val stream = new ByteArrayOutputStream()
assertEquals(0, runFormatCommand(stream, availableDirs))
- val actual = stream.toString().split("\\r?\\n")
+ val actual = stream.toString().split("\\r?\\n").drop(1)
val expect = availableDirs.map("Formatting %s".format(_))
assertEquals(availableDirs.size, actual.size)
Review Comment:
It seems to me this (failed) check can be removed. All we wan to test is the folders get formatted only once, and that is verified in the following assert.
##########
core/src/test/scala/unit/kafka/tools/StorageToolTest.scala:
##########
@@ -194,7 +194,7 @@ Found problem:
val stream2 = new ByteArrayOutputStream()
assertEquals(0, StorageTool.
formatCommand(new PrintStream(stream2), Seq(tempDir.toString), metaProperties, bootstrapMetadata, MetadataVersion.latestTesting(), ignoreFormatted = true))
- assertEquals("All of the log directories are already formatted.%n".format(), stream2.toString())
+ assertEquals("All of the log directories are already formatted.".format(), stream2.toString().split("\\r?\\n").drop(1)(0))
Review Comment:
ditto
##########
core/src/test/scala/unit/kafka/tools/StorageToolTest.scala:
##########
@@ -182,7 +182,7 @@ Found problem:
val bootstrapMetadata = StorageTool.buildBootstrapMetadata(MetadataVersion.latestTesting(), None, "test format command")
assertEquals(0, StorageTool.
formatCommand(new PrintStream(stream), Seq(tempDir.toString), metaProperties, bootstrapMetadata, MetadataVersion.latestTesting(), ignoreFormatted = false))
- assertTrue(stream.toString().startsWith("Formatting %s".format(tempDir)))
+ assertTrue(stream.toString().split("\\r?\\n").drop(1)(0).startsWith("Formatting %s".format(tempDir)))
Review Comment:
Maybe we can check that output contains the expected content. for example:
```scala
assertTrue(stream.toString().split("\\r?\\n").exists(_.startsWith("Formatting %s".format(tempDir))))
```
`drop(1)(0)` is too magic to understand to me :)
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16888 Fix failed StorageToolTest.testFormatSucceedsIfAllDirectoriesAreAvailable and StorageToolTest.testFormatEmptyDirectory [kafka]
Posted by "brandboat (via GitHub)" <gi...@apache.org>.
brandboat commented on PR #16186:
URL: https://github.com/apache/kafka/pull/16186#issuecomment-2146856204
> Thanks for the fix @brandboat ! I assume the patch fixes the broken tests, but could you please add some info in the PR description for reviewers know why this fixes the issue. Something like:
Why it is broken
In which commit changes the behavior and why changes
How do you fix it
Thanks.
Added some description in pr, 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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org