adoroszlai commented on code in PR #7974:
URL: https://github.com/apache/ozone/pull/7974#discussion_r1981887223
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -4480,18 +4480,22 @@ public OzoneLockProvider getOzoneLockProvider() {
}
public ReplicationConfig getDefaultReplicationConfig() {
- if (this.defaultReplicationConfig != null) {
- return this.defaultReplicationConfig;
+ if (defaultReplicationConfig == null) {
+ setReplicationFromConfig();
}
+ return defaultReplicationConfig;
+ }
+ public void setReplicationFromConfig() {
Review Comment:
Thanks @hemantk-12 for the review.
I'm reluctant to add `@VisibleForTesting` for various reasons:
1. Its javadoc says it should not be used on `public` or `protected`
members, since that indicates bad design.
2. It is not validated by any means. If it gets outdated, the annotation is
misleading, like outdated javadocs. Examples: `OzoneManager#getKeyManager()`
is annotated with `@VisibleForTesting`, but it's used from lots of non-test
code outside of the package. `getKmsProvider()`, `getOmStorage()` and
`getOmSnapshotProvider()` are also called from other package non-test code. I
haven't checked all other methods where it's applied, but we see a pattern.
3. More dependence on Guava. (Hadoop even introduced its own annotation for
the same to avoid that.)
4. One more round of CI for such a small change (of questionable value).
Please let me know if you feel strongly about the need to add it.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
