You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "lanshiqin (via GitHub)" <gi...@apache.org> on 2023/04/16 00:13:20 UTC
[GitHub] [kafka] lanshiqin opened a new pull request, #13584: MINOR: Add log segment unit tests, If the maximum offset beyond index, appen…
lanshiqin opened a new pull request, #13584:
URL: https://github.com/apache/kafka/pull/13584
Add log segment unit tests, If the maximum offset beyond index, appended to the log section, it throws LogSegmentOffsetOverflowException
*More detailed description of change
The unit tests for the log segment should cover more scenario validation, and the unit tests submitted this time cover LogSegmentOffsetOverflowException that are expected to be thrown when the maximum index is exceeded
--
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
[GitHub] [kafka] lanshiqin commented on a diff in pull request #13584: MINOR: Add log segment unit tests, If the maximum offset beyond index, appen…
Posted by "lanshiqin (via GitHub)" <gi...@apache.org>.
lanshiqin commented on code in PR #13584:
URL: https://github.com/apache/kafka/pull/13584#discussion_r1169828631
##########
core/src/test/scala/unit/kafka/log/LogSegmentTest.scala:
##########
@@ -65,6 +66,21 @@ class LogSegmentTest {
Utils.delete(logDir)
}
+ /**
+ * If the maximum offset beyond index, appended to the log section, it throws LogSegmentOffsetOverflowException
+ */
+ @Test
+ def testAppendForLogSegmentOffsetOverflowException(): Unit = {
+ val seg = createSegment(0)
+ val largestOffset: Long = Integer.MAX_VALUE + 1
Review Comment:
Good idea
--
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
[GitHub] [kafka] lanshiqin commented on a diff in pull request #13584: MINOR: Add log segment unit tests, If the maximum offset beyond index, appen…
Posted by "lanshiqin (via GitHub)" <gi...@apache.org>.
lanshiqin commented on code in PR #13584:
URL: https://github.com/apache/kafka/pull/13584#discussion_r1169831617
##########
core/src/test/scala/unit/kafka/log/LogSegmentTest.scala:
##########
@@ -65,6 +66,21 @@ class LogSegmentTest {
Utils.delete(logDir)
}
+ /**
+ * If the maximum offset beyond index, appended to the log section, it throws LogSegmentOffsetOverflowException
+ */
+ @Test
+ def testAppendForLogSegmentOffsetOverflowException(): Unit = {
+ val seg = createSegment(0)
Review Comment:
Thanks, good advice, I've modified it along with this scheme
--
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
[GitHub] [kafka] lanshiqin commented on a diff in pull request #13584: MINOR: Add log segment unit tests, If the maximum offset beyond index, appen…
Posted by "lanshiqin (via GitHub)" <gi...@apache.org>.
lanshiqin commented on code in PR #13584:
URL: https://github.com/apache/kafka/pull/13584#discussion_r1191256878
##########
core/src/test/scala/unit/kafka/log/LogSegmentTest.scala:
##########
@@ -65,6 +68,32 @@ class LogSegmentTest {
Utils.delete(logDir)
}
+ /**
+ * LogSegmentOffsetOverflowException should be thrown while appending the logs if:
+ * 1. largestOffset < 0
Review Comment:
Thanks for your advice, I have revised the description.
--
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
[GitHub] [kafka] Hangleton commented on a diff in pull request #13584: MINOR: Add log segment unit tests, If the maximum offset beyond index, appen…
Posted by "Hangleton (via GitHub)" <gi...@apache.org>.
Hangleton commented on code in PR #13584:
URL: https://github.com/apache/kafka/pull/13584#discussion_r1168535982
##########
core/src/test/scala/unit/kafka/log/LogSegmentTest.scala:
##########
@@ -65,6 +66,21 @@ class LogSegmentTest {
Utils.delete(logDir)
}
+ /**
+ * If the maximum offset beyond index, appended to the log section, it throws LogSegmentOffsetOverflowException
+ */
+ @Test
+ def testAppendForLogSegmentOffsetOverflowException(): Unit = {
+ val seg = createSegment(0)
+ val largestOffset: Long = Integer.MAX_VALUE + 1
+ val largestTimestamp: Long = Time.SYSTEM.milliseconds()
+ val shallowOffsetOfMaxTimestamp: Long = 0
+ val memoryRecords: MemoryRecords = records(0, "hello")
+ assertThrows(classOf[LogSegmentOffsetOverflowException], () => {
+ seg.append(largestOffset, largestTimestamp, shallowOffsetOfMaxTimestamp, memoryRecords)
Review Comment:
nit: `largestTimestamp` could be misleading as it could make believe the timestamp itself is set to the maximum possible value.
--
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
[GitHub] [kafka] showuon commented on pull request #13584: MINOR: Add log segment unit tests, If the maximum offset beyond index, appen…
Posted by "showuon (via GitHub)" <gi...@apache.org>.
showuon commented on PR #13584:
URL: https://github.com/apache/kafka/pull/13584#issuecomment-1571202851
@Hangleton , do you have any other comments? If no, I'm going to merge it this week.
--
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
[GitHub] [kafka] Hangleton commented on a diff in pull request #13584: MINOR: Add log segment unit tests, If the maximum offset beyond index, appen…
Posted by "Hangleton (via GitHub)" <gi...@apache.org>.
Hangleton commented on code in PR #13584:
URL: https://github.com/apache/kafka/pull/13584#discussion_r1191598672
##########
core/src/test/scala/unit/kafka/log/LogSegmentTest.scala:
##########
@@ -65,6 +68,32 @@ class LogSegmentTest {
Utils.delete(logDir)
}
+ /**
+ * LogSegmentOffsetOverflowException should be thrown while appending the logs if:
+ * 1. largestOffset < 0
+ * 2. largestOffset - baseOffset < 0
+ * 3. largestOffset - baseOffset > Integer.MAX_VALUE
+ */
+ @ParameterizedTest
+ @CsvSource(Array(
+ "0, -2147483648",
+ "0, 2147483648",
+ "1, 0",
+ "100, 10",
+ "2147483648, 0",
+ "-2147483648, 0",
+ "2147483648,4294967296"
+ ))
+ def testAppendForLogSegmentOffsetOverflowException(baseOffset: Long, largestOffset: Long): Unit = {
Review Comment:
I see, thanks for checking. Could we maybe confirm if the overflow exception comes from this line in `TimeIndex#maybeAppend`:
```
mmap.putInt(relativeOffset(offset))
```
And if that is the case, why is the relative offset < 0 or > max_int?
--
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
[GitHub] [kafka] divijvaidya commented on a diff in pull request #13584: MINOR: Add log segment unit tests, If the maximum offset beyond index, appen…
Posted by "divijvaidya (via GitHub)" <gi...@apache.org>.
divijvaidya commented on code in PR #13584:
URL: https://github.com/apache/kafka/pull/13584#discussion_r1168516608
##########
core/src/test/scala/unit/kafka/log/LogSegmentTest.scala:
##########
@@ -65,6 +66,21 @@ class LogSegmentTest {
Utils.delete(logDir)
}
+ /**
+ * If the maximum offset beyond index, appended to the log section, it throws LogSegmentOffsetOverflowException
+ */
+ @Test
+ def testAppendForLogSegmentOffsetOverflowException(): Unit = {
+ val seg = createSegment(0)
+ val largestOffset: Long = Integer.MAX_VALUE + 1
+ val largestTimestamp: Long = Time.SYSTEM.milliseconds()
Review Comment:
In the spirit of keeping this test run fast and deterministic, could we hard code a value here?
##########
core/src/test/scala/unit/kafka/log/LogSegmentTest.scala:
##########
@@ -65,6 +66,21 @@ class LogSegmentTest {
Utils.delete(logDir)
}
+ /**
+ * If the maximum offset beyond index, appended to the log section, it throws LogSegmentOffsetOverflowException
+ */
+ @Test
+ def testAppendForLogSegmentOffsetOverflowException(): Unit = {
+ val seg = createSegment(0)
+ val largestOffset: Long = Integer.MAX_VALUE + 1
Review Comment:
I would suggest to use `@ParameterizedTest` and also test for:
1. Negative value for largestOffset
2. Value exceeding the range of integer (existing)
##########
core/src/test/scala/unit/kafka/log/LogSegmentTest.scala:
##########
@@ -65,6 +66,21 @@ class LogSegmentTest {
Utils.delete(logDir)
}
+ /**
+ * If the maximum offset beyond index, appended to the log section, it throws LogSegmentOffsetOverflowException
+ */
+ @Test
+ def testAppendForLogSegmentOffsetOverflowException(): Unit = {
+ val seg = createSegment(0)
Review Comment:
May I suggest to create a segment with non-zero index so that we can test for https://github.com/apache/kafka/blob/trunk/storage/src/main/java/org/apache/kafka/storage/internals/log/AbstractIndex.java#L554 condition to be false. The `relativeOffset` should fit within an integer but the current test case only tests for situation where we have 0 as starting index.
--
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
[GitHub] [kafka] Hangleton commented on a diff in pull request #13584: MINOR: Add log segment unit tests, If the maximum offset beyond index, appen…
Posted by "Hangleton (via GitHub)" <gi...@apache.org>.
Hangleton commented on code in PR #13584:
URL: https://github.com/apache/kafka/pull/13584#discussion_r1191598230
##########
core/src/test/scala/unit/kafka/log/LogSegmentTest.scala:
##########
@@ -65,6 +68,32 @@ class LogSegmentTest {
Utils.delete(logDir)
}
+ /**
+ * LogSegmentOffsetOverflowException should be thrown while appending the logs if:
+ * 1. largestOffset < 0
+ * 2. largestOffset - baseOffset < 0
+ * 3. largestOffset - baseOffset > Integer.MAX_VALUE
+ */
+ @ParameterizedTest
+ @CsvSource(Array(
+ "0, -2147483648",
+ "0, 2147483648",
+ "1, 0",
+ "100, 10",
+ "2147483648, 0",
+ "-2147483648, 0",
+ "2147483648,4294967296"
+ ))
+ def testAppendForLogSegmentOffsetOverflowException(baseOffset: Long, largestOffset: Long): Unit = {
Review Comment:
I see, thanks for checking. Could we maybe confirm if the overflow exception comes from this line in `TimeIndex#maybeAppend`:
```
mmap.putInt(relativeOffset(offset))
```
And if that is the case, why is the relative offset < 0 or > max_int?
--
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
[GitHub] [kafka] lanshiqin commented on a diff in pull request #13584: MINOR: Add log segment unit tests, If the maximum offset beyond index, appen…
Posted by "lanshiqin (via GitHub)" <gi...@apache.org>.
lanshiqin commented on code in PR #13584:
URL: https://github.com/apache/kafka/pull/13584#discussion_r1191252780
##########
core/src/test/scala/unit/kafka/log/LogSegmentTest.scala:
##########
@@ -65,6 +68,32 @@ class LogSegmentTest {
Utils.delete(logDir)
}
+ /**
+ * LogSegmentOffsetOverflowException should be thrown while appending the logs if:
+ * 1. largestOffset < 0
+ * 2. largestOffset - baseOffset < 0
+ * 3. largestOffset - baseOffset > Integer.MAX_VALUE
+ */
+ @ParameterizedTest
+ @CsvSource(Array(
+ "0, -2147483648",
+ "0, 2147483648",
+ "1, 0",
+ "100, 10",
+ "2147483648, 0",
+ "-2147483648, 0",
+ "2147483648,4294967296"
Review Comment:
Yes, it was intentionally written as 2147483648. The test method and LogSegment receive offset arguments of type Long in order to test scenarios exceeding Int.MAX_VALUE
--
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
[GitHub] [kafka] showuon commented on a diff in pull request #13584: MINOR: Add log segment unit tests, If the maximum offset beyond index, appen…
Posted by "showuon (via GitHub)" <gi...@apache.org>.
showuon commented on code in PR #13584:
URL: https://github.com/apache/kafka/pull/13584#discussion_r1188050289
##########
core/src/test/scala/unit/kafka/log/LogSegmentTest.scala:
##########
@@ -65,6 +68,29 @@ class LogSegmentTest {
Utils.delete(logDir)
}
+ /**
+ * If the maximum offset beyond index, appended to the log section, it throws LogSegmentOffsetOverflowException
Review Comment:
This comment is not clear enough (ex: what is maximum offset mean in this test?). Maybe:
```
LogSegmentOffsetOverflowException should be thrown while appending the logs if:
1. largestOffset - baseOffset < 0
2. largestOffset - baseOffset > Integer.MAX_VALUE
```
--
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
[GitHub] [kafka] lanshiqin commented on a diff in pull request #13584: MINOR: Add log segment unit tests, If the maximum offset beyond index, appen…
Posted by "lanshiqin (via GitHub)" <gi...@apache.org>.
lanshiqin commented on code in PR #13584:
URL: https://github.com/apache/kafka/pull/13584#discussion_r1191984789
##########
core/src/test/scala/unit/kafka/log/LogSegmentTest.scala:
##########
@@ -65,6 +68,32 @@ class LogSegmentTest {
Utils.delete(logDir)
}
+ /**
+ * LogSegmentOffsetOverflowException should be thrown while appending the logs if:
+ * 1. largestOffset < 0
+ * 2. largestOffset - baseOffset < 0
+ * 3. largestOffset - baseOffset > Integer.MAX_VALUE
+ */
+ @ParameterizedTest
+ @CsvSource(Array(
+ "0, -2147483648",
+ "0, 2147483648",
+ "1, 0",
+ "100, 10",
+ "2147483648, 0",
+ "-2147483648, 0",
+ "2147483648,4294967296"
+ ))
+ def testAppendForLogSegmentOffsetOverflowException(baseOffset: Long, largestOffset: Long): Unit = {
Review Comment:
Sorry, I need to correct that.
When baseOffset == largestOffset, LogSegment.append executes normally without throwing an exception. Because my unit test of parameters shallowOffsetOfMaxTimestamp value is 0, which can lead to rear unit test processing, TimeIndex# maybeAppend: In a `relativeOffset(offset),` the value of offset is 0. If baseOffset is a value greater than 0, the result will be <0, resulting in an exception.
I have repaired the shallowOffsetOfMaxTimestamp value of this test.
--
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
[GitHub] [kafka] lanshiqin commented on a diff in pull request #13584: MINOR: Add log segment unit tests, If the maximum offset beyond index, appen…
Posted by "lanshiqin (via GitHub)" <gi...@apache.org>.
lanshiqin commented on code in PR #13584:
URL: https://github.com/apache/kafka/pull/13584#discussion_r1170685549
##########
core/src/test/scala/unit/kafka/log/LogSegmentTest.scala:
##########
@@ -65,6 +68,28 @@ class LogSegmentTest {
Utils.delete(logDir)
}
+ /**
+ * If the maximum offset beyond index, appended to the log section, it throws LogSegmentOffsetOverflowException
+ */
+ @ParameterizedTest
+ @CsvSource(Array(
Review Comment:
Thanks, I've added this use case data
--
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
[GitHub] [kafka] showuon commented on a diff in pull request #13584: MINOR: Add log segment unit tests, If the maximum offset beyond index, appen…
Posted by "showuon (via GitHub)" <gi...@apache.org>.
showuon commented on code in PR #13584:
URL: https://github.com/apache/kafka/pull/13584#discussion_r1188050289
##########
core/src/test/scala/unit/kafka/log/LogSegmentTest.scala:
##########
@@ -65,6 +68,29 @@ class LogSegmentTest {
Utils.delete(logDir)
}
+ /**
+ * If the maximum offset beyond index, appended to the log section, it throws LogSegmentOffsetOverflowException
Review Comment:
This comment is not clear enough (ex: what is maximum offset mean in this test? what index mean here?). Maybe:
```
LogSegmentOffsetOverflowException should be thrown while appending the logs if:
1. largestOffset - baseOffset < 0
2. largestOffset - baseOffset > Integer.MAX_VALUE
```
--
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
[GitHub] [kafka] lanshiqin commented on a diff in pull request #13584: MINOR: Add log segment unit tests, If the maximum offset beyond index, appen…
Posted by "lanshiqin (via GitHub)" <gi...@apache.org>.
lanshiqin commented on code in PR #13584:
URL: https://github.com/apache/kafka/pull/13584#discussion_r1188451651
##########
core/src/test/scala/unit/kafka/log/LogSegmentTest.scala:
##########
@@ -65,6 +68,29 @@ class LogSegmentTest {
Utils.delete(logDir)
}
+ /**
+ * If the maximum offset beyond index, appended to the log section, it throws LogSegmentOffsetOverflowException
Review Comment:
Good idea. I've modified it
--
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
[GitHub] [kafka] lanshiqin commented on a diff in pull request #13584: MINOR: Add log segment unit tests, If the maximum offset beyond index, appen…
Posted by "lanshiqin (via GitHub)" <gi...@apache.org>.
lanshiqin commented on code in PR #13584:
URL: https://github.com/apache/kafka/pull/13584#discussion_r1169710407
##########
core/src/test/scala/unit/kafka/log/LogSegmentTest.scala:
##########
@@ -65,6 +66,21 @@ class LogSegmentTest {
Utils.delete(logDir)
}
+ /**
+ * If the maximum offset beyond index, appended to the log section, it throws LogSegmentOffsetOverflowException
+ */
+ @Test
+ def testAppendForLogSegmentOffsetOverflowException(): Unit = {
+ val seg = createSegment(0)
+ val largestOffset: Long = Integer.MAX_VALUE + 1
+ val largestTimestamp: Long = Time.SYSTEM.milliseconds()
+ val shallowOffsetOfMaxTimestamp: Long = 0
+ val memoryRecords: MemoryRecords = records(0, "hello")
+ assertThrows(classOf[LogSegmentOffsetOverflowException], () => {
+ seg.append(largestOffset, largestTimestamp, shallowOffsetOfMaxTimestamp, memoryRecords)
Review Comment:
Thanks, I should have changed the variable name to the 'currentTime'
--
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
[GitHub] [kafka] lanshiqin commented on a diff in pull request #13584: MINOR: Add log segment unit tests, If the maximum offset beyond index, appen…
Posted by "lanshiqin (via GitHub)" <gi...@apache.org>.
lanshiqin commented on code in PR #13584:
URL: https://github.com/apache/kafka/pull/13584#discussion_r1169715205
##########
core/src/test/scala/unit/kafka/log/LogSegmentTest.scala:
##########
@@ -65,6 +66,21 @@ class LogSegmentTest {
Utils.delete(logDir)
}
+ /**
+ * If the maximum offset beyond index, appended to the log section, it throws LogSegmentOffsetOverflowException
+ */
+ @Test
+ def testAppendForLogSegmentOffsetOverflowException(): Unit = {
+ val seg = createSegment(0)
+ val largestOffset: Long = Integer.MAX_VALUE + 1
+ val largestTimestamp: Long = Time.SYSTEM.milliseconds()
Review Comment:
Thanks, it really shouldn't be hardcoded directly here, I should use @ParameterizedTest
--
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
[GitHub] [kafka] lanshiqin commented on pull request #13584: MINOR: Add log segment unit tests, If the maximum offset beyond index, appen…
Posted by "lanshiqin (via GitHub)" <gi...@apache.org>.
lanshiqin commented on PR #13584:
URL: https://github.com/apache/kafka/pull/13584#issuecomment-1512838660
Use @ParameterizedTest more test scenarios are covered.
1. Negative value for largestOffset
2. Value exceeding the range of integer
3. Create a segment with non-zero index so that we can test for https://github.com/apache/kafka/blob/trunk/storage/src/main/java/org/apache/kafka/storage/internals/log/AbstractIndex.java#L554 condition to be false.
--
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
[GitHub] [kafka] lanshiqin commented on a diff in pull request #13584: MINOR: Add log segment unit tests, If the maximum offset beyond index, appen…
Posted by "lanshiqin (via GitHub)" <gi...@apache.org>.
lanshiqin commented on code in PR #13584:
URL: https://github.com/apache/kafka/pull/13584#discussion_r1191248219
##########
core/src/test/scala/unit/kafka/log/LogSegmentTest.scala:
##########
@@ -65,6 +68,32 @@ class LogSegmentTest {
Utils.delete(logDir)
}
+ /**
+ * LogSegmentOffsetOverflowException should be thrown while appending the logs if:
+ * 1. largestOffset < 0
+ * 2. largestOffset - baseOffset < 0
+ * 3. largestOffset - baseOffset > Integer.MAX_VALUE
+ */
+ @ParameterizedTest
+ @CsvSource(Array(
+ "0, -2147483648",
+ "0, 2147483648",
+ "1, 0",
+ "100, 10",
+ "2147483648, 0",
+ "-2147483648, 0",
+ "2147483648,4294967296"
+ ))
+ def testAppendForLogSegmentOffsetOverflowException(baseOffset: Long, largestOffset: Long): Unit = {
Review Comment:
When baseOffset == largestOffset, the teardown method in the test class is called at the end of the test, which in turn calls the logSegment _.close() => timeIndex.maybeAppend, Eventually an exception will be thrown (Integer overflow for offset: 0), but this exception is not the point that this test method needs to cover
--
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
[GitHub] [kafka] Hangleton commented on a diff in pull request #13584: MINOR: Add log segment unit tests, If the maximum offset beyond index, appen…
Posted by "Hangleton (via GitHub)" <gi...@apache.org>.
Hangleton commented on code in PR #13584:
URL: https://github.com/apache/kafka/pull/13584#discussion_r1212777323
##########
core/src/test/scala/unit/kafka/log/LogSegmentTest.scala:
##########
@@ -65,6 +68,32 @@ class LogSegmentTest {
Utils.delete(logDir)
}
+ /**
+ * LogSegmentOffsetOverflowException should be thrown while appending the logs if:
+ * 1. largestOffset < 0
+ * 2. largestOffset - baseOffset < 0
+ * 3. largestOffset - baseOffset > Integer.MAX_VALUE
+ */
+ @ParameterizedTest
+ @CsvSource(Array(
+ "0, -2147483648",
+ "0, 2147483648",
+ "1, 0",
+ "100, 10",
+ "2147483648, 0",
+ "-2147483648, 0",
+ "2147483648,4294967296"
+ ))
+ def testAppendForLogSegmentOffsetOverflowException(baseOffset: Long, largestOffset: Long): Unit = {
Review Comment:
Got it, thanks for confirming!
--
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
[GitHub] [kafka] divijvaidya commented on a diff in pull request #13584: MINOR: Add log segment unit tests, If the maximum offset beyond index, appen…
Posted by "divijvaidya (via GitHub)" <gi...@apache.org>.
divijvaidya commented on code in PR #13584:
URL: https://github.com/apache/kafka/pull/13584#discussion_r1170238988
##########
core/src/test/scala/unit/kafka/log/LogSegmentTest.scala:
##########
@@ -65,6 +68,28 @@ class LogSegmentTest {
Utils.delete(logDir)
}
+ /**
+ * If the maximum offset beyond index, appended to the log section, it throws LogSegmentOffsetOverflowException
+ */
+ @ParameterizedTest
+ @CsvSource(Array(
Review Comment:
Note that baseOffset can be `Long` but `largestOffset - baseOffset` should be <= `Integer.MaxValue`. This case is missing from our test here.
Could we add the following test cases as well:
baseOffset is a number > Integer.MaxValue, largestOffset is a number > Integer.MaxValue, such that `largestOffset - baseOffset` > Integer.MaxValue. This will throw an error.
--
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
[GitHub] [kafka] Hangleton commented on a diff in pull request #13584: MINOR: Add log segment unit tests, If the maximum offset beyond index, appen…
Posted by "Hangleton (via GitHub)" <gi...@apache.org>.
Hangleton commented on code in PR #13584:
URL: https://github.com/apache/kafka/pull/13584#discussion_r1188635591
##########
core/src/test/scala/unit/kafka/log/LogSegmentTest.scala:
##########
@@ -65,6 +68,32 @@ class LogSegmentTest {
Utils.delete(logDir)
}
+ /**
+ * LogSegmentOffsetOverflowException should be thrown while appending the logs if:
+ * 1. largestOffset < 0
Review Comment:
Nit: we can remove (1) since (1) => (2).
##########
core/src/test/scala/unit/kafka/log/LogSegmentTest.scala:
##########
@@ -65,6 +68,32 @@ class LogSegmentTest {
Utils.delete(logDir)
}
+ /**
+ * LogSegmentOffsetOverflowException should be thrown while appending the logs if:
+ * 1. largestOffset < 0
+ * 2. largestOffset - baseOffset < 0
+ * 3. largestOffset - baseOffset > Integer.MAX_VALUE
+ */
+ @ParameterizedTest
+ @CsvSource(Array(
+ "0, -2147483648",
+ "0, 2147483648",
+ "1, 0",
+ "100, 10",
+ "2147483648, 0",
+ "-2147483648, 0",
+ "2147483648,4294967296"
Review Comment:
nit: Note that the max int value on the Java platform is `2147483647` so we are exercising max int + 1.
##########
core/src/test/scala/unit/kafka/log/LogSegmentTest.scala:
##########
@@ -65,6 +68,32 @@ class LogSegmentTest {
Utils.delete(logDir)
}
+ /**
+ * LogSegmentOffsetOverflowException should be thrown while appending the logs if:
+ * 1. largestOffset < 0
+ * 2. largestOffset - baseOffset < 0
+ * 3. largestOffset - baseOffset > Integer.MAX_VALUE
+ */
+ @ParameterizedTest
+ @CsvSource(Array(
+ "0, -2147483648",
+ "0, 2147483648",
+ "1, 0",
+ "100, 10",
+ "2147483648, 0",
+ "-2147483648, 0",
+ "2147483648,4294967296"
+ ))
+ def testAppendForLogSegmentOffsetOverflowException(baseOffset: Long, largestOffset: Long): Unit = {
Review Comment:
What is the expected behaviour for `baseOffset == largestOffset`?
--
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
[GitHub] [kafka] showuon merged pull request #13584: MINOR: Add log segment unit tests, If the maximum offset beyond index, appen…
Posted by "showuon (via GitHub)" <gi...@apache.org>.
showuon merged PR #13584:
URL: https://github.com/apache/kafka/pull/13584
--
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
[GitHub] [kafka] Hangleton commented on pull request #13584: MINOR: Add log segment unit tests, If the maximum offset beyond index, appen…
Posted by "Hangleton (via GitHub)" <gi...@apache.org>.
Hangleton commented on PR #13584:
URL: https://github.com/apache/kafka/pull/13584#issuecomment-1571584730
@showuon Thanks for the follow-up and apologies for the delay, the PR seems good 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