[GitHub] [kafka] pprovenzano commented on pull request #13513: KAFKA-14881: Update UserScramCredentialRecord for SCRAM ZK to KRaft migration

["pprovenzano (via GitHub)" <gi...@apache.org>]

pprovenzano commented on PR #13513:
URL: https://github.com/apache/kafka/pull/13513#issuecomment-1500649607

   > Thanks for this, @pprovenzano . Seems like a relatively straightforward change. However, you need to give a reason why we should do this. I think the reason here is for compatibility with SCRAM credentials migrated from ZK, right? (This should be in the change description)
   > 
   > The current comments in `UserScramCredentialRecord.json` for `StoredKey` and `ServerKey` are not helpful. They just repeat the name of the field. If I understand correctly the meanings should be something like: `StoredKey: used by the server to verify the client's identity` and `ServerKey: used by the server to prove its identity to the client` ?
   > 
   > It seems that `ScramControlManager.toRecord` is no longer used; can we remove it?
   > 
   > Can we have a `ScramCredentialDataTest` that tests `ScramCredentialData.toRecord` / `ScramCredentialData.fromRecord` ?
   
   Thank you for the review. I've updated the PR with all of your suggestions. 
   


-- 
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