MINOR: cleanup core modules part 1#15252
Conversation
Mark methods and fields private where possible Annotate public methods and fields Remove unused classes and methods
Mark methods and fields private where possible Annotate public methods and fields Remove unused classes and methods
|
Failures seem all unrelated to the changes |
showuon
left a comment
showuon
left a comment
There was a problem hiding this comment.
Thanks for the PR. Left some comments.
| } catch { | ||
| case e: Exception => { | ||
| case e: Exception => | ||
| e.printStackTrace() | ||
| // must exit with non-zero status so system tests will know we failed | ||
| Exit.exit(1) | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm not completely sure if it'll work as before after removing the curly brackets. Could you confirm it?
There was a problem hiding this comment.
From: https://docs.scala-lang.org/style/control-structures.html#curlybraces
case - Always omit braces in case clauses.
I also double checked with one of the most used Scala code bases in concurrent world (Pekko, fork of Akka) that this is the case. You can see this in this class for example (one of the key classes of Pekko):
https://github.com/apache/incubator-pekko/blob/e597a702b9a54a0e193e09a50a1d45dfd1f56785/actor/src/main/scala/org/apache/pekko/actor/ActorCell.scala#L659
This is an instance where Java and Scala differ (in Java this diff would cause a different behaviour indeed).
| case class ClientIdAllBrokers(clientId: String) extends ClientIdBroker { | ||
| override def toString = "%s-%s".format(clientId, "AllBrokers") |
There was a problem hiding this comment.
So ClientIdAllBrokers is unused class. Good catch!
Signed-off-by: Josep Prat <josep.prat@aiven.io>
|
All tests failures seem to be unrelated to these changes. |
* MINOR: Clean up core api, cluster, common, log, admin, controller and coordinator classes Mark methods and fields private where possible Annotate public methods and fields Remove unused classes and methods Signed-off-by: Josep Prat <josep.prat@aiven.io> Reviewers: Luke Chen <showuon@gmail.com>
* MINOR: Clean up core api, cluster, common, log, admin, controller and coordinator classes Mark methods and fields private where possible Annotate public methods and fields Remove unused classes and methods Signed-off-by: Josep Prat <josep.prat@aiven.io> Reviewers: Luke Chen <showuon@gmail.com>
* MINOR: Clean up core api, cluster, common, log, admin, controller and coordinator classes Mark methods and fields private where possible Annotate public methods and fields Remove unused classes and methods Signed-off-by: Josep Prat <josep.prat@aiven.io> Reviewers: Luke Chen <showuon@gmail.com>
2 participants
I'm going through the core classes and found some warnings that could be cleaned up, this is the first batch of clean ups.
Even if the code needs to be ported to Java, this might help in the process (clearer types, less code, right visibility).
This PR cleans up: admin, api, cluster, common, controller, coordinator and log classes:
More detailed description of your change,
if necessary. The PR title and PR message become
the squashed commit message, so use a separate
comment to ping reviewers.
Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.
Committer Checklist (excluded from commit message)