adoroszlai commented on code in PR #7130:
URL: https://github.com/apache/ozone/pull/7130#discussion_r1735932507
##########
hadoop-ozone/dist/src/main/compose/xcompat/secure-new-cluster.yaml:
##########
Review Comment:
Instead of adding entirely new cluster and config and test script, how about
changing the existing one to secure? I think it would be useful to test key /
fs operations with security enabled, plus it would reduce duplication.
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
@@ -2595,8 +2595,15 @@ public OzoneFsServerDefaults getServerDefaults() throws
IOException {
@Override
public URI getKeyProviderUri() throws IOException {
- return OzoneKMSUtil.getKeyProviderUri(ugi,
- null, getServerDefaults().getKeyProviderUri(), conf);
+ if (omVersion.compareTo(OzoneManagerVersion.SERVER_DEFAULTS) >= 0) {
+ try {
+ return OzoneKMSUtil.getKeyProviderUri(ugi,
+ null, getServerDefaults().getKeyProviderUri(), conf);
+ } catch (Exception e) {
+ LOG.warn("Could not get key provider URI from OM.", e);
+ }
+ }
Review Comment:
I think it would be better to change `getServerDefaults()` to handle the
"old OM" and error cases, in order to simplify life for callers of
`getServerDefaults()`.
I'd also consider changing its return type (and `serverDefaults`) to
`Optional<OzoneFsServerDefaults>`.
Something like this:
```java
@Override
public Optional<OzoneFsServerDefaults> getServerDefaults() throws
IOException {
if (omVersion.compareTo(OzoneManagerVersion.SERVER_DEFAULTS) < 0) {
return Optional.empty();
}
long now = Time.monotonicNow();
if (!serverDefaults.isPresent() ||
(now - serverDefaultsLastUpdate > serverDefaultsValidityPeriod)) {
try {
serverDefaults = Optional.of(ozoneManagerClient.getServerDefaults());
serverDefaultsLastUpdate = now;
} catch (Exception e) {
LOG.warn("Could not get server defaults from OM.", e);
}
}
return serverDefaults;
}
@Override
public URI getKeyProviderUri() throws IOException {
String keyProviderUri =
serverDefaults.map(FsServerDefaults::getKeyProviderUri)
.orElse(null);
return OzoneKMSUtil.getKeyProviderUri(ugi, null, keyProviderUri, conf);
}
```
--
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]
