[GitHub] [kafka] cmccabe commented on pull request #13180: MINOR: Add a summary of the metadata migration

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

cmccabe commented on PR #13180:
URL: https://github.com/apache/kafka/pull/13180#issuecomment-1412624413

   Thanks for the PR. WIth regard to the formatting changes, can we use the style we've been doing elsewhere in the scala code of:
   ```
   functionName(
     parameterA: typeA,
     parameterB: typeB,
   ...
   ): Unit = {
   ...
   }
   ```
   I find it more readable than the style where we have lots of indenting
   ```
   functionName(parameterA: typeA,
                parameterB: typeB,
   ...
   ): Unit = {
   ...
   }
   ```
   
   Can we have `MigrationSummary` be `MigrationManifest`? And be immutable and have equals(), hashCode() and all that for the sake of unit tests. That does imply we'd need a `MigrationManifest.Builder` or something, but that seems OK?
   
   I think `toSummaryString` really would be better off as a method inside `MetadataImage`. We are not going to remember to update it if it's stored off the side. It would also be good to be clearer what is coming from the image and what from the delta. Like `123 topics (5 changed)` or something...


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