errose28 commented on code in PR #11789:
URL: https://github.com/apache/ozone/pull/11789#discussion_r4415517766
##########
hadoop-ozone/client/src/test/java/org/apache/hadoop/ozone/client/rpc/TestRpcClient.java:
##########
@@ -362,6 +374,174 @@ public void testGetOmVersionWithFutureOM() {
om(OzoneManagerVersion.UNKNOWN_VERSION))));
}
+ @ParameterizedTest
+ @ValueSource(booleans = {false, true})
+ void testOmVersionRefreshesOnDemandAfterExpiry(boolean customInterval)
throws Exception {
+ OzoneConfiguration config = new OzoneConfiguration();
+ if (customInterval) {
+ config.set(OZONE_CLIENT_OM_VERSION_REFRESH_INTERVAL, "37s");
+ }
+ long interval = config.getTimeDuration(
+ OZONE_CLIENT_OM_VERSION_REFRESH_INTERVAL,
OZONE_CLIENT_OM_VERSION_REFRESH_INTERVAL_DEFAULT,
+ TimeUnit.MILLISECONDS);
+ VersionedOmTransport transport = new VersionedOmTransport();
+ try (MockedStatic<Time> time = mockStatic(Time.class)) {
Review Comment:
Instead of `MockedStatic` (for single thread testing) and `FieldUtils` (for
multi-thread testing), it would be cleaner to create a constructor overload for
the `RpcClient` that takes a `Clock` and uses that for the interval checks.
Tests would then pass `MockClock` for this.
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
@@ -2974,6 +2984,20 @@ public KeyProvider call() throws Exception {
@Override
public OzoneManagerVersion getOmVersion() {
+ long lastUpdate = omVersionLastUpdate.get();
+ if (lastUpdate == OM_VERSION_REFRESHING || Time.monotonicNow() -
lastUpdate < omVersionRefreshInterval) {
+ return omVersion;
+ }
+ if (omVersionLastUpdate.compareAndSet(lastUpdate, OM_VERSION_REFRESHING)) {
+ try {
+ omVersion = getOmVersion(ozoneManagerClient.getServiceInfo());
+ } catch (IOException e) {
Review Comment:
To make this truly transparent to the operation that checked the version,
any exception that occurs should cause us to fall back to the last version.
```suggestion
} catch (Exception e) {
```
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
@@ -2974,6 +2984,20 @@ public KeyProvider call() throws Exception {
@Override
public OzoneManagerVersion getOmVersion() {
+ long lastUpdate = omVersionLastUpdate.get();
+ if (lastUpdate == OM_VERSION_REFRESHING || Time.monotonicNow() -
lastUpdate < omVersionRefreshInterval) {
+ return omVersion;
+ }
+ if (omVersionLastUpdate.compareAndSet(lastUpdate, OM_VERSION_REFRESHING)) {
+ try {
+ omVersion = getOmVersion(ozoneManagerClient.getServiceInfo());
Review Comment:
Kind of an edge case, but if the leader returns an empty OM list in the
response for some reason, `getOmVersion` will default to the client's largest
known OM version. We can make this continue to use the last known OM version
instead.
--
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]
