You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "divijvaidya (via GitHub)" <gi...@apache.org> on 2023/06/29 17:11:40 UTC
[GitHub] [kafka] divijvaidya opened a new pull request, #13936: MINOR: Refactor & cleanup for RemoteIndexCache
divijvaidya opened a new pull request, #13936:
URL: https://github.com/apache/kafka/pull/13936
**Changes**
1. Add new unit tests
2. Change the on-disk filename from `<offset>_<uuid>_.<indexSuffix>` to `<offset>_<uuid>.<indexSuffix>` i.e. remove trailing suffix
3. Fix a small bug where we were parsing offset as Int when reading the file name from disk. Offset is long.
4. Perform input validation in RemoteLogSegmentMetadata.
5. Remove an extra loop in cleaner thread. Shutdownable thread already performs looping.
--
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 pull request #13936: MINOR: Refactor & cleanup for RemoteIndexCache
Posted by "divijvaidya (via GitHub)" <gi...@apache.org>.
divijvaidya commented on PR #13936:
URL: https://github.com/apache/kafka/pull/13936#issuecomment-1627102695
Unrelated test failures in CI. Merging this one.
--
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] satishd commented on a diff in pull request #13936: MINOR: Refactor & cleanup for RemoteIndexCache
Posted by "satishd (via GitHub)" <gi...@apache.org>.
satishd commented on code in PR #13936:
URL: https://github.com/apache/kafka/pull/13936#discussion_r1256079424
##########
storage/api/src/main/java/org/apache/kafka/server/log/remote/storage/RemoteLogSegmentMetadata.java:
##########
@@ -100,7 +100,16 @@ public RemoteLogSegmentMetadata(RemoteLogSegmentId remoteLogSegmentId,
this.remoteLogSegmentId = Objects.requireNonNull(remoteLogSegmentId, "remoteLogSegmentId can not be null");
this.state = Objects.requireNonNull(state, "state can not be null");
+ if (startOffset < 0) {
+ throw new IllegalArgumentException(
+ String.format("Unexpected start offset = %d. StartOffset for a remote segment cannot negative", startOffset));
Review Comment:
Why `String.format` is used here and in the below exception cases?
##########
storage/api/src/main/java/org/apache/kafka/server/log/remote/storage/RemoteLogSegmentMetadata.java:
##########
@@ -100,7 +100,16 @@ public RemoteLogSegmentMetadata(RemoteLogSegmentId remoteLogSegmentId,
this.remoteLogSegmentId = Objects.requireNonNull(remoteLogSegmentId, "remoteLogSegmentId can not be null");
this.state = Objects.requireNonNull(state, "state can not be null");
+ if (startOffset < 0) {
+ throw new IllegalArgumentException(
+ String.format("Unexpected start offset = %d. StartOffset for a remote segment cannot negative", startOffset));
Review Comment:
typo: "remote segment cannot negative" -> "remote segment can not be negative"
--
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 #13936: MINOR: Refactor & cleanup for RemoteIndexCache
Posted by "divijvaidya (via GitHub)" <gi...@apache.org>.
divijvaidya commented on code in PR #13936:
URL: https://github.com/apache/kafka/pull/13936#discussion_r1255777691
##########
storage/api/src/main/java/org/apache/kafka/server/log/remote/storage/RemoteLogSegmentMetadata.java:
##########
@@ -100,7 +100,16 @@ public RemoteLogSegmentMetadata(RemoteLogSegmentId remoteLogSegmentId,
this.remoteLogSegmentId = Objects.requireNonNull(remoteLogSegmentId, "remoteLogSegmentId can not be null");
this.state = Objects.requireNonNull(state, "state can not be null");
+ if (startOffset < 0) {
+ throw new IllegalArgumentException(
+ String.format("Unexpected start offset = %d. StartOffset for a remote segment cannot negative", startOffset));
+ }
this.startOffset = startOffset;
+
+ if (endOffset < 0) {
+ throw new IllegalArgumentException(
+ String.format("Unexpected end offset = %d. EndOffset for a remote segment cannot negative", endOffset));
+ }
Review Comment:
fixed in latest commit
--
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] jeqo commented on a diff in pull request #13936: MINOR: Refactor & cleanup for RemoteIndexCache
Posted by "jeqo (via GitHub)" <gi...@apache.org>.
jeqo commented on code in PR #13936:
URL: https://github.com/apache/kafka/pull/13936#discussion_r1255748772
##########
storage/api/src/main/java/org/apache/kafka/server/log/remote/storage/RemoteLogSegmentMetadata.java:
##########
@@ -100,7 +100,16 @@ public RemoteLogSegmentMetadata(RemoteLogSegmentId remoteLogSegmentId,
this.remoteLogSegmentId = Objects.requireNonNull(remoteLogSegmentId, "remoteLogSegmentId can not be null");
this.state = Objects.requireNonNull(state, "state can not be null");
+ if (startOffset < 0) {
+ throw new IllegalArgumentException(
+ String.format("Unexpected start offset = %d. StartOffset for a remote segment cannot negative", startOffset));
+ }
this.startOffset = startOffset;
+
+ if (endOffset < 0) {
+ throw new IllegalArgumentException(
+ String.format("Unexpected end offset = %d. EndOffset for a remote segment cannot negative", endOffset));
+ }
Review Comment:
Maybe?
```suggestion
if (endOffset < startOffset) {
throw new IllegalArgumentException(
String.format("Unexpected end offset = %d. EndOffset for a remote segment cannot be less than startOffset {}", endOffset, startOffset));
}
```
--
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] satishd commented on a diff in pull request #13936: MINOR: Refactor & cleanup for RemoteIndexCache
Posted by "satishd (via GitHub)" <gi...@apache.org>.
satishd commented on code in PR #13936:
URL: https://github.com/apache/kafka/pull/13936#discussion_r1255603306
##########
core/src/main/scala/kafka/log/remote/RemoteIndexCache.scala:
##########
@@ -38,7 +38,60 @@ import java.util.concurrent.locks.ReentrantReadWriteLock
object RemoteIndexCache {
val DirName = "remote-log-index-cache"
val TmpFileSuffix = ".tmp"
- val remoteLogIndexCacheCleanerThread = "remote-log-index-cleaner"
+ val RemoteLogIndexCacheCleanerThread = "remote-log-index-cleaner"
+
+ def RemoteLogSegmentIdFromRemoteIndexFileName(fileName: String): Uuid = {
Review Comment:
It looks like we do not use `PascalCase or CamelCase` for methods in object class. Should we continue using `camelCase` here like in other places in this project?
--
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] satishd commented on pull request #13936: MINOR: Refactor & cleanup for RemoteIndexCache
Posted by "satishd (via GitHub)" <gi...@apache.org>.
satishd commented on PR #13936:
URL: https://github.com/apache/kafka/pull/13936#issuecomment-1619565860
Thanks @divijvaidya for the PR, let me know when it is ready for review.
--
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 merged pull request #13936: MINOR: Refactor & cleanup for RemoteIndexCache
Posted by "divijvaidya (via GitHub)" <gi...@apache.org>.
divijvaidya merged PR #13936:
URL: https://github.com/apache/kafka/pull/13936
--
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 #13936: MINOR: Refactor & cleanup for RemoteIndexCache
Posted by "divijvaidya (via GitHub)" <gi...@apache.org>.
divijvaidya commented on code in PR #13936:
URL: https://github.com/apache/kafka/pull/13936#discussion_r1255713117
##########
core/src/main/scala/kafka/log/remote/RemoteIndexCache.scala:
##########
@@ -38,7 +38,60 @@ import java.util.concurrent.locks.ReentrantReadWriteLock
object RemoteIndexCache {
val DirName = "remote-log-index-cache"
val TmpFileSuffix = ".tmp"
- val remoteLogIndexCacheCleanerThread = "remote-log-index-cleaner"
+ val RemoteLogIndexCacheCleanerThread = "remote-log-index-cleaner"
+
+ def RemoteLogSegmentIdFromRemoteIndexFileName(fileName: String): Uuid = {
Review Comment:
fixed in latest commit
--
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 pull request #13936: MINOR: Refactor & cleanup for RemoteIndexCache
Posted by "divijvaidya (via GitHub)" <gi...@apache.org>.
divijvaidya commented on PR #13936:
URL: https://github.com/apache/kafka/pull/13936#issuecomment-1623577849
@satishd this is ready for review.
--
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 #13936: MINOR: Refactor & cleanup for RemoteIndexCache
Posted by "divijvaidya (via GitHub)" <gi...@apache.org>.
divijvaidya commented on code in PR #13936:
URL: https://github.com/apache/kafka/pull/13936#discussion_r1256126637
##########
storage/api/src/main/java/org/apache/kafka/server/log/remote/storage/RemoteLogSegmentMetadata.java:
##########
@@ -100,7 +100,16 @@ public RemoteLogSegmentMetadata(RemoteLogSegmentId remoteLogSegmentId,
this.remoteLogSegmentId = Objects.requireNonNull(remoteLogSegmentId, "remoteLogSegmentId can not be null");
this.state = Objects.requireNonNull(state, "state can not be null");
+ if (startOffset < 0) {
+ throw new IllegalArgumentException(
+ String.format("Unexpected start offset = %d. StartOffset for a remote segment cannot negative", startOffset));
Review Comment:
fixed
##########
storage/api/src/main/java/org/apache/kafka/server/log/remote/storage/RemoteLogSegmentMetadata.java:
##########
@@ -100,7 +100,16 @@ public RemoteLogSegmentMetadata(RemoteLogSegmentId remoteLogSegmentId,
this.remoteLogSegmentId = Objects.requireNonNull(remoteLogSegmentId, "remoteLogSegmentId can not be null");
this.state = Objects.requireNonNull(state, "state can not be null");
+ if (startOffset < 0) {
+ throw new IllegalArgumentException(
+ String.format("Unexpected start offset = %d. StartOffset for a remote segment cannot negative", startOffset));
Review Comment:
this is fixed in latest commit
--
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 #13936: MINOR: Refactor & cleanup for RemoteIndexCache
Posted by "divijvaidya (via GitHub)" <gi...@apache.org>.
divijvaidya commented on code in PR #13936:
URL: https://github.com/apache/kafka/pull/13936#discussion_r1256109183
##########
storage/api/src/main/java/org/apache/kafka/server/log/remote/storage/RemoteLogSegmentMetadata.java:
##########
@@ -100,7 +100,16 @@ public RemoteLogSegmentMetadata(RemoteLogSegmentId remoteLogSegmentId,
this.remoteLogSegmentId = Objects.requireNonNull(remoteLogSegmentId, "remoteLogSegmentId can not be null");
this.state = Objects.requireNonNull(state, "state can not be null");
+ if (startOffset < 0) {
+ throw new IllegalArgumentException(
+ String.format("Unexpected start offset = %d. StartOffset for a remote segment cannot negative", startOffset));
Review Comment:
No specific reason. I think it's visually more clear to read the log string part together instead of adding a `+`. But I don't have strong opinion about it. I will change to append if that's what you prefer.
--
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