You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "showuon (via GitHub)" <gi...@apache.org> on 2023/09/07 09:06:28 UTC
[GitHub] [kafka] showuon opened a new pull request, #14352: MINOR: update the javadoc in RSM
showuon opened a new pull request, #14352:
URL: https://github.com/apache/kafka/pull/14352
Update the javadoc in `RemoteStorageManager#fetchIndex` to guide the providers how to implement it.
### 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
[GitHub] [kafka] showuon commented on a diff in pull request #14352: MINOR: update the javadoc in RSM
Posted by "showuon (via GitHub)" <gi...@apache.org>.
showuon commented on code in PR #14352:
URL: https://github.com/apache/kafka/pull/14352#discussion_r1318426217
##########
storage/api/src/main/java/org/apache/kafka/server/log/remote/storage/RemoteStorageManager.java:
##########
@@ -121,14 +121,17 @@ InputStream fetchLogSegment(RemoteLogSegmentMetadata remoteLogSegmentMetadata,
/**
* Returns the index for the respective log segment of {@link RemoteLogSegmentMetadata}.
+ * <p>
+ * Note: The transaction index may not exist because segments created prior to version 2.8.0 or non-transactional topics...etc.
+ * In this case, it should still return an InputStream with empty content, instead of returning {@code null}.
Review Comment:
Changed to `The transaction index may not exist because of non-transactional topics.`. 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
[GitHub] [kafka] showuon commented on a diff in pull request #14352: MINOR: update the javadoc in RSM
Posted by "showuon (via GitHub)" <gi...@apache.org>.
showuon commented on code in PR #14352:
URL: https://github.com/apache/kafka/pull/14352#discussion_r1318426217
##########
storage/api/src/main/java/org/apache/kafka/server/log/remote/storage/RemoteStorageManager.java:
##########
@@ -121,14 +121,17 @@ InputStream fetchLogSegment(RemoteLogSegmentMetadata remoteLogSegmentMetadata,
/**
* Returns the index for the respective log segment of {@link RemoteLogSegmentMetadata}.
+ * <p>
+ * Note: The transaction index may not exist because segments created prior to version 2.8.0 or non-transactional topics...etc.
+ * In this case, it should still return an InputStream with empty content, instead of returning {@code null}.
Review Comment:
Changed to `The transaction index may not exist because of no transactional records.`. 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
[GitHub] [kafka] satishd commented on pull request #14352: MINOR: update the javadoc in RSM
Posted by "satishd (via GitHub)" <gi...@apache.org>.
satishd commented on PR #14352:
URL: https://github.com/apache/kafka/pull/14352#issuecomment-1710350430
There are a few test failures unrelated to this change. Merging it to trunk and 3.6.
--
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 #14352: MINOR: update the javadoc in RSM
Posted by "showuon (via GitHub)" <gi...@apache.org>.
showuon commented on code in PR #14352:
URL: https://github.com/apache/kafka/pull/14352#discussion_r1318425718
##########
storage/api/src/main/java/org/apache/kafka/server/log/remote/storage/RemoteStorageManager.java:
##########
@@ -121,14 +121,17 @@ InputStream fetchLogSegment(RemoteLogSegmentMetadata remoteLogSegmentMetadata,
/**
* Returns the index for the respective log segment of {@link RemoteLogSegmentMetadata}.
+ * <p>
+ * Note: The transaction index may not exist because segments created prior to version 2.8.0 or non-transactional topics...etc.
+ * In this case, it should still return an InputStream with empty content, instead of returning {@code null}.
*
* @param remoteLogSegmentMetadata metadata about the remote log segment.
* @param indexType type of the index to be fetched for the segment.
* @return input stream of the requested index.
* @throws RemoteStorageException if there are any errors while fetching the index.
* @throws RemoteResourceNotFoundException the requested index is not found in the remote storage
- * (e.g. Transaction index may not exist because segments created prior to version 2.8.0 will not have transaction index associated with them.).
- * The caller of this function are encouraged to re-create the indexes from the segment
+ *
+ * The implementation of this function are encouraged to re-create the indexes from the segment
* as the suggested way of handling this error if the index is expected to be existed.
Review Comment:
Removed.
--
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 #14352: MINOR: update the javadoc in RSM
Posted by "satishd (via GitHub)" <gi...@apache.org>.
satishd commented on code in PR #14352:
URL: https://github.com/apache/kafka/pull/14352#discussion_r1318410493
##########
storage/api/src/main/java/org/apache/kafka/server/log/remote/storage/RemoteStorageManager.java:
##########
@@ -121,14 +121,17 @@ InputStream fetchLogSegment(RemoteLogSegmentMetadata remoteLogSegmentMetadata,
/**
* Returns the index for the respective log segment of {@link RemoteLogSegmentMetadata}.
+ * <p>
+ * Note: The transaction index may not exist because segments created prior to version 2.8.0 or non-transactional topics...etc.
+ * In this case, it should still return an InputStream with empty content, instead of returning {@code null}.
*
* @param remoteLogSegmentMetadata metadata about the remote log segment.
* @param indexType type of the index to be fetched for the segment.
* @return input stream of the requested index.
* @throws RemoteStorageException if there are any errors while fetching the index.
* @throws RemoteResourceNotFoundException the requested index is not found in the remote storage
- * (e.g. Transaction index may not exist because segments created prior to version 2.8.0 will not have transaction index associated with them.).
- * The caller of this function are encouraged to re-create the indexes from the segment
+ *
+ * The implementation of this function are encouraged to re-create the indexes from the segment
* as the suggested way of handling this error if the index is expected to be existed.
Review Comment:
+1 to remove it avoiding confusion.
##########
storage/api/src/main/java/org/apache/kafka/server/log/remote/storage/RemoteStorageManager.java:
##########
@@ -121,14 +121,17 @@ InputStream fetchLogSegment(RemoteLogSegmentMetadata remoteLogSegmentMetadata,
/**
* Returns the index for the respective log segment of {@link RemoteLogSegmentMetadata}.
+ * <p>
+ * Note: The transaction index may not exist because segments created prior to version 2.8.0 or non-transactional topics...etc.
+ * In this case, it should still return an InputStream with empty content, instead of returning {@code null}.
Review Comment:
As the javadoc says it needs to return an empty input-stream.
--
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 #14352: MINOR: update the javadoc in RSM
Posted by "showuon (via GitHub)" <gi...@apache.org>.
showuon commented on code in PR #14352:
URL: https://github.com/apache/kafka/pull/14352#discussion_r1318317138
##########
storage/api/src/main/java/org/apache/kafka/server/log/remote/storage/RemoteStorageManager.java:
##########
@@ -121,14 +121,17 @@ InputStream fetchLogSegment(RemoteLogSegmentMetadata remoteLogSegmentMetadata,
/**
* Returns the index for the respective log segment of {@link RemoteLogSegmentMetadata}.
+ * <p>
+ * Note: The transaction index may not exist because segments created prior to version 2.8.0 or non-transactional topics...etc.
+ * In this case, it should still return an InputStream with empty content, instead of returning {@code null}.
*
* @param remoteLogSegmentMetadata metadata about the remote log segment.
* @param indexType type of the index to be fetched for the segment.
* @return input stream of the requested index.
* @throws RemoteStorageException if there are any errors while fetching the index.
* @throws RemoteResourceNotFoundException the requested index is not found in the remote storage
- * (e.g. Transaction index may not exist because segments created prior to version 2.8.0 will not have transaction index associated with them.).
- * The caller of this function are encouraged to re-create the indexes from the segment
Review Comment:
I don't believe we expect the _caller_ do anything after getting the index, since the caller are all internal methods.
--
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] kamalcph commented on a diff in pull request #14352: MINOR: update the javadoc in RSM
Posted by "kamalcph (via GitHub)" <gi...@apache.org>.
kamalcph commented on code in PR #14352:
URL: https://github.com/apache/kafka/pull/14352#discussion_r1318418335
##########
storage/api/src/main/java/org/apache/kafka/server/log/remote/storage/RemoteStorageManager.java:
##########
@@ -121,14 +121,17 @@ InputStream fetchLogSegment(RemoteLogSegmentMetadata remoteLogSegmentMetadata,
/**
* Returns the index for the respective log segment of {@link RemoteLogSegmentMetadata}.
+ * <p>
+ * Note: The transaction index may not exist because segments created prior to version 2.8.0 or non-transactional topics...etc.
+ * In this case, it should still return an InputStream with empty content, instead of returning {@code null}.
*
* @param remoteLogSegmentMetadata metadata about the remote log segment.
* @param indexType type of the index to be fetched for the segment.
* @return input stream of the requested index.
* @throws RemoteStorageException if there are any errors while fetching the index.
* @throws RemoteResourceNotFoundException the requested index is not found in the remote storage
- * (e.g. Transaction index may not exist because segments created prior to version 2.8.0 will not have transaction index associated with them.).
- * The caller of this function are encouraged to re-create the indexes from the segment
Review Comment:
Good to remove this line.
--
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 #14352: MINOR: update the javadoc in RSM
Posted by "showuon (via GitHub)" <gi...@apache.org>.
showuon commented on code in PR #14352:
URL: https://github.com/apache/kafka/pull/14352#discussion_r1318551914
##########
storage/api/src/main/java/org/apache/kafka/server/log/remote/storage/RemoteStorageManager.java:
##########
@@ -121,14 +121,17 @@ InputStream fetchLogSegment(RemoteLogSegmentMetadata remoteLogSegmentMetadata,
/**
* Returns the index for the respective log segment of {@link RemoteLogSegmentMetadata}.
+ * <p>
+ * Note: The transaction index may not exist because segments created prior to version 2.8.0 or non-transactional topics...etc.
+ * In this case, it should still return an InputStream with empty content, instead of returning {@code null}.
Review Comment:
Added a note in [KAFKA-14993](https://issues.apache.org/jira/browse/KAFKA-14993):
`Note, please remember to update the javadoc in RSM after the fix. See: https://github.com/apache/kafka/pull/14352`
Thanks for pointing to the ticket.
--
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 #14352: MINOR: update the javadoc in RSM
Posted by "showuon (via GitHub)" <gi...@apache.org>.
showuon commented on PR #14352:
URL: https://github.com/apache/kafka/pull/14352#issuecomment-1709782685
@kamalcph @satishd , call for review. 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
[GitHub] [kafka] showuon commented on a diff in pull request #14352: MINOR: update the javadoc in RSM
Posted by "showuon (via GitHub)" <gi...@apache.org>.
showuon commented on code in PR #14352:
URL: https://github.com/apache/kafka/pull/14352#discussion_r1318317947
##########
storage/api/src/main/java/org/apache/kafka/server/log/remote/storage/RemoteStorageManager.java:
##########
@@ -121,14 +121,17 @@ InputStream fetchLogSegment(RemoteLogSegmentMetadata remoteLogSegmentMetadata,
/**
* Returns the index for the respective log segment of {@link RemoteLogSegmentMetadata}.
+ * <p>
+ * Note: The transaction index may not exist because segments created prior to version 2.8.0 or non-transactional topics...etc.
+ * In this case, it should still return an InputStream with empty content, instead of returning {@code null}.
*
* @param remoteLogSegmentMetadata metadata about the remote log segment.
* @param indexType type of the index to be fetched for the segment.
* @return input stream of the requested index.
* @throws RemoteStorageException if there are any errors while fetching the index.
* @throws RemoteResourceNotFoundException the requested index is not found in the remote storage
- * (e.g. Transaction index may not exist because segments created prior to version 2.8.0 will not have transaction index associated with them.).
- * The caller of this function are encouraged to re-create the indexes from the segment
+ *
+ * The implementation of this function are encouraged to re-create the indexes from the segment
* as the suggested way of handling this error if the index is expected to be existed.
Review Comment:
Is this true? Should we remove them?
--
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] kamalcph commented on a diff in pull request #14352: MINOR: update the javadoc in RSM
Posted by "kamalcph (via GitHub)" <gi...@apache.org>.
kamalcph commented on code in PR #14352:
URL: https://github.com/apache/kafka/pull/14352#discussion_r1318413354
##########
storage/api/src/main/java/org/apache/kafka/server/log/remote/storage/RemoteStorageManager.java:
##########
@@ -121,14 +121,17 @@ InputStream fetchLogSegment(RemoteLogSegmentMetadata remoteLogSegmentMetadata,
/**
* Returns the index for the respective log segment of {@link RemoteLogSegmentMetadata}.
+ * <p>
+ * Note: The transaction index may not exist because segments created prior to version 2.8.0 or non-transactional topics...etc.
+ * In this case, it should still return an InputStream with empty content, instead of returning {@code null}.
Review Comment:
Not clear on this line:
>The transaction index may not exist because segments created prior to version 2.8.0
Only the producer snapshots are not expected prior v2.8.0 not the transaction index. Correct me if I'm wrong.
--
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 #14352: MINOR: update the javadoc in RSM
Posted by "showuon (via GitHub)" <gi...@apache.org>.
showuon commented on code in PR #14352:
URL: https://github.com/apache/kafka/pull/14352#discussion_r1318319429
##########
storage/api/src/main/java/org/apache/kafka/server/log/remote/storage/RemoteStorageManager.java:
##########
@@ -121,14 +121,17 @@ InputStream fetchLogSegment(RemoteLogSegmentMetadata remoteLogSegmentMetadata,
/**
* Returns the index for the respective log segment of {@link RemoteLogSegmentMetadata}.
+ * <p>
+ * Note: The transaction index may not exist because segments created prior to version 2.8.0 or non-transactional topics...etc.
+ * In this case, it should still return an InputStream with empty content, instead of returning {@code null}.
Review Comment:
Encountered an error with a RSM returning `null`, since the indexCache will assert not null 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] satishd commented on a diff in pull request #14352: MINOR: update the javadoc in RSM
Posted by "satishd (via GitHub)" <gi...@apache.org>.
satishd commented on code in PR #14352:
URL: https://github.com/apache/kafka/pull/14352#discussion_r1318539662
##########
storage/api/src/main/java/org/apache/kafka/server/log/remote/storage/RemoteStorageManager.java:
##########
@@ -121,14 +121,17 @@ InputStream fetchLogSegment(RemoteLogSegmentMetadata remoteLogSegmentMetadata,
/**
* Returns the index for the respective log segment of {@link RemoteLogSegmentMetadata}.
+ * <p>
+ * Note: The transaction index may not exist because segments created prior to version 2.8.0 or non-transactional topics...etc.
+ * In this case, it should still return an InputStream with empty content, instead of returning {@code null}.
Review Comment:
Right, transaction index can be null, but there is an existing https://issues.apache.org/jira/browse/KAFKA-14993 to address that.
--
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 merged pull request #14352: MINOR: update the javadoc in RSM
Posted by "satishd (via GitHub)" <gi...@apache.org>.
satishd merged PR #14352:
URL: https://github.com/apache/kafka/pull/14352
--
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