[GitHub] [kafka] jsancio commented on pull request #10593: KAFKA-10800 Validate the snapshot id when the state machine creates a snapshot

2021-06-14 Thread GitBox 


jsancio commented on pull request #10593:
URL: https://github.com/apache/kafka/pull/10593#issuecomment-860980428


   > Or may be we can make a seperate PR to handle the validation for 
onSnapshotFrozen?
   
   I am okay with fixing this in a future PR. Do you want to go ahead and file 
an sub-task for https://issues.apache.org/jira/browse/KAFKA-10310 and link it 
here?


-- 
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:
us...@infra.apache.org

[GitHub] [kafka] jsancio commented on pull request #10593: KAFKA-10800 Validate the snapshot id when the state machine creates a snapshot

2021-06-14 Thread GitBox 


jsancio commented on pull request #10593:
URL: https://github.com/apache/kafka/pull/10593#issuecomment-860908546


   > I assume a KIP is needed.
   
   We don't need a KIP. All of these API are internal APIs that are not 
accessible/publish to projects external to Apache Kafka.


-- 
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:
us...@infra.apache.org

[GitHub] [kafka] jsancio commented on pull request #10593: KAFKA-10800 Validate the snapshot id when the state machine creates a snapshot

2021-06-10 Thread GitBox 


jsancio commented on pull request #10593:
URL: https://github.com/apache/kafka/pull/10593#issuecomment-858925958


   > @jsancio Can you share more details about the possible concurrency 
scenario with createSnapshot ? BTW, will moving the validation to 
onSnapshotFrozen imply that before creating the snapshot, there's no 
validation? I think maybe we can keep the validation here and add some 
additional check before freeze() which makes the snapshot visible?
   
   @feyman2016, I think it is reasonable to do both. Validate when 
`createSnapshot` is called and validate again in `onSnapshotFrozen`. In both 
cases this validation should be optional. Validate if it is created through 
`RaftClient.createSnapshot`. Don't validate if `KafkaRaftClient` creates the 
snapshot internally because of a `FetchResponse` from the leader.
   
   I have been working on a PR related to this if you want to take a look: 
https://github.com/apache/kafka/pull/10786. It would be nice to get your PR 
merged before my PR.


-- 
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:
us...@infra.apache.org

[GitHub] [kafka] jsancio commented on pull request #10593: KAFKA-10800 Validate the snapshot id when the state machine creates a snapshot

2021-06-01 Thread GitBox 


jsancio commented on pull request #10593:
URL: https://github.com/apache/kafka/pull/10593#issuecomment-852353685


   > Are there any concerns with multi-threading here? I think the answer is 
"no". Any thoughts @feyman2016 or @jsancio?
   
   Good point. There may be some concurrency issue with `createSnapshot`. How 
about moving the validation to `onSnapshotFrozen`?
   
   This conversation also reminded me to create this issue: 
https://issues.apache.org/jira/browse/KAFKA-12873. It is slightly related to 
this point.


-- 
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:
us...@infra.apache.org

[GitHub] [kafka] jsancio commented on pull request #10593: KAFKA-10800 Validate the snapshot id when the state machine creates a snapshot

2021-05-27 Thread GitBox 


jsancio commented on pull request #10593:
URL: https://github.com/apache/kafka/pull/10593#issuecomment-849787584


   > @jsancio Addressed the comments but just found that I would need to 
generalize context.advanceLeaderHighWatermarkToEndOffset (), will update later~
   
   I needed something similar in a PR I am currently working and this is what I 
have if you want to adapt it to your PR:
   
   ```java
   public void advanceLocalLeaderHighWatermarkToLogEndOffset() throws 
InterruptedException {
   assertEquals(localId, currentLeader());
   long localLogEndOffset = log.endOffset().offset;
   Set followers = voters.stream().filter(voter -> voter != 
localId.getAsInt()).collect(Collectors.toSet());
   
   // Send a request from every follower
   for (int follower : followers) {
   deliverRequest(
   fetchRequest(currentEpoch(), follower, localLogEndOffset, 
currentEpoch(), 0)
   );
   pollUntilResponse();
   assertSentFetchPartitionResponse(Errors.NONE, currentEpoch(), 
localId);
   }
   
   pollUntil(() -> 
OptionalLong.of(localLogEndOffset).equals(client.highWatermark()));
   }
   ```


-- 
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:
us...@infra.apache.org