adoroszlai commented on a change in pull request #3031: URL: https://github.com/apache/ozone/pull/3031#discussion_r802025180
########## File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OMProtocolVersion.java ########## @@ -0,0 +1,64 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * <p> + * http://www.apache.org/licenses/LICENSE-2.0 + * <p> + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hadoop.ozone; + +import org.apache.hadoop.util.ComparableVersion; + +/** + * Versioning for OM's APIs used by ozone clients. + */ +public final class OMProtocolVersion { + + // OM Authenticates the user using the S3 Auth info embedded in request proto. + public static final String OM_SUPPORTS_S3AUTH_VIA_PROTO = "2.0.0"; + public static final String OM_SUPPORTS_EC = "2.1.0"; + public static final String OM_SUPPORTS_FSO = "2.1.0"; Review comment: What's the benefit of having a 3-digit version number? When would we increment each of the digits? ########## File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/ClientVersions.java ########## @@ -28,8 +28,27 @@ // DatanodeDetails#getFromProtobuf handles unknown types of ports public static final int VERSION_HANDLES_UNKNOWN_DN_PORTS = 1; + // TODO: Change the version of FSO/EC if one is released before the other. + // Client supports EC objects + public static final int CLIENT_EC_CAPABLE = 2; + // Client supports FSO buckets + public static final int CLIENT_FSO_CAPABLE = CLIENT_EC_CAPABLE; + // this should always point to the latest version - public static final int CURRENT_VERSION = VERSION_HANDLES_UNKNOWN_DN_PORTS; + public static final int CURRENT_VERSION = CLIENT_EC_CAPABLE; + + /** + * Validates if the client version is equal to or newer than the expected + * version supported. Client are expected to be backward compatible if it + * is sending a request. + * @param desiredClientVersion Minimum version of the client expected. + * @param actualClientVersion Version of the client witnessed. + * @return true if client is at or newer than the desired version. + */ + public static boolean isClientCompatible(int desiredClientVersion, + int actualClientVersion) { + return desiredClientVersion >= actualClientVersion; Review comment: I think "client version is equal to or newer" is opposite of `desiredClientVersion >= actualClientVersion`. ```suggestion return desiredClientVersion <= actualClientVersion; ``` ########## File path: hadoop-hdds/common/src/test/java/org/apache/hadoop/OMProtocolVersionTest.java ########## @@ -0,0 +1,85 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * <p> + * http://www.apache.org/licenses/LICENSE-2.0 + * <p> + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hadoop; + +import org.junit.jupiter.api.Test; + +import java.util.LinkedList; +import java.util.List; + +import static org.apache.hadoop.ozone.OMProtocolVersion.OM_LATEST_VERSION; + +class OMProtocolVersionTest { + private enum ValidateOmVersionTestCases { + NULL_EXPECTED_NULL_OM( + null, // Expected version + null, // OM Version + true), // Should validation pass + EMPTY_EXPECTED_NULL_OM( + "", + null, + true), + EMPTY_EXPECTED_EMPTY_OM( + "", + "", + true), + NULL_EXPECTED_EMPTY_OM( + null, + "", + true), + OM_EXPECTED_LATEST_OM( + "1.0.0", + OM_LATEST_VERSION, + true), + OM_EXPECTED_NEWER_OM( + "1.1.0", + "1.11.0", + true), + NEWER_EXPECTED_OLD_OM( + "1.20.0", + "1.19.0", + false), + NULL_EXPECTED_OM( + null, + OM_LATEST_VERSION, + true); + private static final List<ValidateOmVersionTestCases> + TEST_CASES = new LinkedList<>(); + + static { + for (ValidateOmVersionTestCases t : values()) { + TEST_CASES.add(t); + } + } + private final String expectedVersion; + private final String omVersion; + private final boolean validation; + ValidateOmVersionTestCases(String expectedVersion, + String omVersion, + boolean validation) { + this.expectedVersion = expectedVersion; + this.omVersion = omVersion; + this.validation = validation; + } + + } + @Test + void isOMCompatible() { + } Review comment: This seems to be incomplete. ########## File path: hadoop-hdds/common/src/test/java/org/apache/hadoop/OMProtocolVersionTest.java ########## @@ -0,0 +1,85 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * <p> + * http://www.apache.org/licenses/LICENSE-2.0 + * <p> + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hadoop; + +import org.junit.jupiter.api.Test; + +import java.util.LinkedList; +import java.util.List; + +import static org.apache.hadoop.ozone.OMProtocolVersion.OM_LATEST_VERSION; + +class OMProtocolVersionTest { Review comment: Ozone build expects tests to be named `Test*`, not `*Test`. So this class and `RpcClientTest` are not run at all in CI. -- 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]
