You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2021/02/10 12:12:20 UTC
[GitHub] [kafka] dengziming opened a new pull request #10097: MINOR: Add FetchSnapshot API doc in KafkaRaftClient
dengziming opened a new pull request #10097:
URL: https://github.com/apache/kafka/pull/10097
*More detailed description of your change*
Currently, we have added FetchSnapshot API in raft, so improve the docs in `KafkaRaftClient`
### 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dengziming commented on a change in pull request #10097: MINOR: Add FetchSnapshot API doc in KafkaRaftClient
Posted by GitBox <gi...@apache.org>.
dengziming commented on a change in pull request #10097:
URL: https://github.com/apache/kafka/pull/10097#discussion_r574955279
##########
File path: raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java
##########
@@ -126,9 +126,16 @@
* gracefully resign from the current epoch. This causes remaining voters to immediately
* begin a new election.
*
- * 4) {@link FetchRequestData}: This is the same as the usual Fetch API in Kafka, but we piggyback
- * some additional metadata on responses (i.e. current leader and epoch). Unlike partition replication,
- * we also piggyback truncation detection on this API rather than through a separate truncation state.
+ * 4) {@link FetchRequestData}: This is the same as the usual Fetch API in Kafka, but we add snapshot
+ * check before responding, and we also piggyback some additional metadata on responses (i.e. current
+ * leader and epoch). Unlike partition replication, we also piggyback truncation detection on this API
+ * rather than through a separate truncation state.
+ *
+ * 5) {@link FetchSnapshotRequestData}: Sent by the follower to the epoch leader to fetch snapshot when
+ * FetchResponse include a snapshot id, this happens when the follower's log end offset is less than
+ * the leader's log start offset. This is similar to the Fetch API since the snapshot is also stored
+ * as FileRecords, but we use {@link UnalignedRecords} in FetchSnapshotResponse because the records is
+ * not necessarily offset-aligned.
Review comment:
Thank you for the reword, Done.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mumrah merged pull request #10097: MINOR: Add FetchSnapshot API doc in KafkaRaftClient
Posted by GitBox <gi...@apache.org>.
mumrah merged pull request #10097:
URL: https://github.com/apache/kafka/pull/10097
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mumrah commented on a change in pull request #10097: MINOR: Add FetchSnapshot API doc in KafkaRaftClient
Posted by GitBox <gi...@apache.org>.
mumrah commented on a change in pull request #10097:
URL: https://github.com/apache/kafka/pull/10097#discussion_r574630121
##########
File path: raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java
##########
@@ -126,9 +126,16 @@
* gracefully resign from the current epoch. This causes remaining voters to immediately
* begin a new election.
*
- * 4) {@link FetchRequestData}: This is the same as the usual Fetch API in Kafka, but we piggyback
- * some additional metadata on responses (i.e. current leader and epoch). Unlike partition replication,
- * we also piggyback truncation detection on this API rather than through a separate truncation state.
+ * 4) {@link FetchRequestData}: This is the same as the usual Fetch API in Kafka, but we add snapshot
+ * check before responding, and we also piggyback some additional metadata on responses (i.e. current
+ * leader and epoch). Unlike partition replication, we also piggyback truncation detection on this API
+ * rather than through a separate truncation state.
+ *
+ * 5) {@link FetchSnapshotRequestData}: Sent by the follower to the epoch leader to fetch snapshot when
+ * FetchResponse include a snapshot id, this happens when the follower's log end offset is less than
+ * the leader's log start offset. This is similar to the Fetch API since the snapshot is also stored
+ * as FileRecords, but we use {@link UnalignedRecords} in FetchSnapshotResponse because the records is
+ * not necessarily offset-aligned.
Review comment:
```suggestion
* 5) {@link FetchSnapshotRequestData}: Sent by the follower to the epoch leader in order to fetch a snapshot.
* This happens when a FetchResponse includes a snapshot ID due to the follower's log end offset being less
* than the leader's log start offset. This API is similar to the Fetch API since the snapshot is stored
* as FileRecords, but we use {@link UnalignedRecords} in FetchSnapshotResponse because the records
* are not necessarily offset-aligned.
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dengziming commented on a change in pull request #10097: MINOR: Add FetchSnapshot API doc in KafkaRaftClient
Posted by GitBox <gi...@apache.org>.
dengziming commented on a change in pull request #10097:
URL: https://github.com/apache/kafka/pull/10097#discussion_r574204983
##########
File path: raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java
##########
@@ -126,9 +126,14 @@
* gracefully resign from the current epoch. This causes remaining voters to immediately
* begin a new election.
*
- * 4) {@link FetchRequestData}: This is the same as the usual Fetch API in Kafka, but we piggyback
- * some additional metadata on responses (i.e. current leader and epoch). Unlike partition replication,
- * we also piggyback truncation detection on this API rather than through a separate truncation state.
+ * 4) {@link FetchRequestData}: This is the same as the usual Fetch API in Kafka, but we add snapshot
+ * check before responding, and we also piggyback some additional metadata on responses (i.e. current
+ * leader and epoch). Unlike partition replication, we also piggyback truncation detection on this API
+ * rather than through a separate truncation state.
+ *
+ * 5) {@link FetchSnapshotRequestData}: Sent by the follower when FetchResponse include a snapshot id,
+ * this is similar to the Fetch API since the snapshot is also stored as FileRecords, but we use
+ * {@link UnalignedRecords} in FetchSnapshotResponse because the records is not necessarily offset-aligned.
Review comment:
Thank you for your advice, Done.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dengziming commented on pull request #10097: MINOR: Add FetchSnapshot API doc in KafkaRaftClient
Posted by GitBox <gi...@apache.org>.
dengziming commented on pull request #10097:
URL: https://github.com/apache/kafka/pull/10097#issuecomment-776667671
@jsancio @hachikuji , trivial changes, PTAL.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jsancio commented on a change in pull request #10097: MINOR: Add FetchSnapshot API doc in KafkaRaftClient
Posted by GitBox <gi...@apache.org>.
jsancio commented on a change in pull request #10097:
URL: https://github.com/apache/kafka/pull/10097#discussion_r574082327
##########
File path: raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java
##########
@@ -126,9 +126,14 @@
* gracefully resign from the current epoch. This causes remaining voters to immediately
* begin a new election.
*
- * 4) {@link FetchRequestData}: This is the same as the usual Fetch API in Kafka, but we piggyback
- * some additional metadata on responses (i.e. current leader and epoch). Unlike partition replication,
- * we also piggyback truncation detection on this API rather than through a separate truncation state.
+ * 4) {@link FetchRequestData}: This is the same as the usual Fetch API in Kafka, but we add snapshot
+ * check before responding, and we also piggyback some additional metadata on responses (i.e. current
+ * leader and epoch). Unlike partition replication, we also piggyback truncation detection on this API
+ * rather than through a separate truncation state.
+ *
+ * 5) {@link FetchSnapshotRequestData}: Sent by the follower when FetchResponse include a snapshot id,
+ * this is similar to the Fetch API since the snapshot is also stored as FileRecords, but we use
+ * {@link UnalignedRecords} in FetchSnapshotResponse because the records is not necessarily offset-aligned.
Review comment:
Thanks for doing this. I also noticed it was missing and fixed it in this PR: https://github.com/apache/kafka/pull/10085/files#diff-1da15c51e641ea46ea5c86201ab8f21cfee9e7c575102a39c7bae0d5ffd7de39R134-R137
Maybe reconcile the two changes and we can merge this one.
##########
File path: raft/src/main/java/org/apache/kafka/raft/NetworkChannel.java
##########
@@ -30,10 +30,8 @@
int newCorrelationId();
/**
- * Send an outbound message. This could be either an outbound request
+ * Send an outbound message. This could be an outbound request
* (i.e. an instance of {@link org.apache.kafka.raft.RaftRequest.Outbound})
- * or a response to a request that was received through {@link #receive(long)}
Review comment:
How about just:
```java
/**
* Send an outbound request message.
*
* @param request outbound request to send
*/
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dengziming commented on a change in pull request #10097: MINOR: Add FetchSnapshot API doc in KafkaRaftClient
Posted by GitBox <gi...@apache.org>.
dengziming commented on a change in pull request #10097:
URL: https://github.com/apache/kafka/pull/10097#discussion_r574955279
##########
File path: raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java
##########
@@ -126,9 +126,16 @@
* gracefully resign from the current epoch. This causes remaining voters to immediately
* begin a new election.
*
- * 4) {@link FetchRequestData}: This is the same as the usual Fetch API in Kafka, but we piggyback
- * some additional metadata on responses (i.e. current leader and epoch). Unlike partition replication,
- * we also piggyback truncation detection on this API rather than through a separate truncation state.
+ * 4) {@link FetchRequestData}: This is the same as the usual Fetch API in Kafka, but we add snapshot
+ * check before responding, and we also piggyback some additional metadata on responses (i.e. current
+ * leader and epoch). Unlike partition replication, we also piggyback truncation detection on this API
+ * rather than through a separate truncation state.
+ *
+ * 5) {@link FetchSnapshotRequestData}: Sent by the follower to the epoch leader to fetch snapshot when
+ * FetchResponse include a snapshot id, this happens when the follower's log end offset is less than
+ * the leader's log start offset. This is similar to the Fetch API since the snapshot is also stored
+ * as FileRecords, but we use {@link UnalignedRecords} in FetchSnapshotResponse because the records is
+ * not necessarily offset-aligned.
Review comment:
Thank you for the reword, Done!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dengziming commented on a change in pull request #10097: MINOR: Add FetchSnapshot API doc in KafkaRaftClient
Posted by GitBox <gi...@apache.org>.
dengziming commented on a change in pull request #10097:
URL: https://github.com/apache/kafka/pull/10097#discussion_r573679151
##########
File path: raft/src/main/java/org/apache/kafka/raft/NetworkChannel.java
##########
@@ -30,10 +30,8 @@
int newCorrelationId();
/**
- * Send an outbound message. This could be either an outbound request
+ * Send an outbound message. This could be an outbound request
* (i.e. an instance of {@link org.apache.kafka.raft.RaftRequest.Outbound})
- * or a response to a request that was received through {@link #receive(long)}
Review comment:
The receive() has been removed in #9732.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org