You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2020/09/04 05:42:27 UTC
[GitHub] [hadoop-ozone] avijayanhwx opened a new pull request #1393: HDDS-4143. Implement a factory for OM Requests that returns an instance based on layout version.
avijayanhwx opened a new pull request #1393:
URL: https://github.com/apache/hadoop-ozone/pull/1393
## What changes were proposed in this pull request?
- Implement Generic factory which stores different instances of Type 'T' sharded by a key & version. A single key can be associated with different versions of 'T'. This is to support a typical use case during upgrade to have multiple versions of a class / method / object and chose them based on current layout version at runtime. Before finalizing, an older version is typically needed, and after finalize, a newer version is needed.
- Using the generic factory, we scan all the different OM "write" requests and associate them with versions.
- Persist current MLV to Ratis requests.
- Layout feature code refactoring. Added more comments and tests.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-4141
https://issues.apache.org/jira/browse/HDDS-4143
## How was this patch tested?
Unit tested.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] linyiqun commented on a change in pull request #1393: HDDS-4143. Implement a factory for OM Requests that returns an instance based on layout version.
Posted by GitBox <gi...@apache.org>.
linyiqun commented on a change in pull request #1393:
URL: https://github.com/apache/hadoop-ozone/pull/1393#discussion_r484082776
##########
File path: hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/upgrade/TestOmRequestFactory.java
##########
@@ -0,0 +1,116 @@
+/**
+ * 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.om.upgrade;
+
+import static org.apache.hadoop.ozone.om.upgrade.OMLayoutFeature.CREATE_EC;
+import static org.apache.hadoop.ozone.om.upgrade.OMLayoutFeature.INITIAL_VERSION;
+import static org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.Type.CreateKey;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.lang.reflect.Modifier;
+import java.util.Set;
+
+import org.apache.hadoop.ozone.om.OMStorage;
+import org.apache.hadoop.ozone.om.exceptions.OMException;
+import org.apache.hadoop.ozone.om.request.OMClientRequest;
+import org.apache.hadoop.ozone.om.request.key.OMECKeyCreateRequest;
+import org.apache.hadoop.ozone.om.request.key.OMKeyCreateRequest;
+import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.LayoutVersion;
+import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.OMRequest;
+import org.junit.Assert;
+import org.junit.BeforeClass;
+import org.junit.Test;
+import org.reflections.Reflections;
+
+/**
+ * Test OmVersionFactory.
+ */
+public class TestOmRequestFactory {
+
+ private static OmRequestFactory omRequestFactory;
+ private static OMLayoutVersionManager omVersionManager;
+
+ @BeforeClass
+ public static void setup() throws OMException {
+ OMStorage omStorage = mock(OMStorage.class);
+ when(omStorage.getLayoutVersion()).thenReturn(0);
+ omVersionManager = OMLayoutVersionManager.initialize(omStorage);
+ omRequestFactory = omVersionManager.getVersionFactory();
+ }
+
+ @Test
+ public void testKeyCreateRequest() {
+
+ // Try getting v1 of 'CreateKey'.
+ Class<? extends OMClientRequest> requestType =
+ omRequestFactory.getRequestType(OMRequest.newBuilder()
+ .setCmdType(CreateKey)
+ .setClientId("c1")
+ .setLayoutVersion(LayoutVersion
+ .newBuilder()
+ .setVersion(INITIAL_VERSION.layoutVersion())
+ .build())
+ .build());
+ Assert.assertEquals(requestType, OMKeyCreateRequest.class);
+
+ // Try getting 'CreateECKey' (V2). Should fail.
+ try {
+ omRequestFactory.getRequestType(OMRequest.newBuilder()
+ .setCmdType(CreateKey)
+ .setClientId("c1")
+ .setLayoutVersion(LayoutVersion
+ .newBuilder()
+ .setVersion(CREATE_EC.layoutVersion())
+ .build())
+ .build());
+ Assert.fail();
+ } catch (IllegalArgumentException ex) {
Review comment:
Can we do the verify for error message that contained in IllegalArgumentException? This will let us know why failure was happened.
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/protocolPB/OzoneManagerRequestHandler.java
##########
@@ -290,6 +292,11 @@ public void validateRequest(OMRequest omRequest) throws OMException {
throw new OMException("ClientId is null",
OMException.ResultCodes.INVALID_REQUEST);
}
+
+ if (omRequest.getLayoutVersion() == null) {
Review comment:
Would be better to add a comment here to explain why we do layout version empty check on handling write request.
##########
File path: hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/upgrade/TestOMVersionManager.java
##########
@@ -54,11 +61,39 @@ public void testOMLayoutVersionManager() throws IOException {
assertEquals(2, omVersionManager.getMetadataLayoutVersion());
}
+ @Test
+ public void testOMLayoutVersionManagerInitError() {
+ OMStorage omStorage = mock(OMStorage.class);
+ when(omStorage.getLayoutVersion()).thenReturn(
+ OMLayoutFeature.values()[OMLayoutFeature.values().length - 1]
+ .layoutVersion() + 1);
+ try {
+ OMLayoutVersionManager.initialize(omStorage);
+ Assert.fail();
+ } catch (OMException ex) {
+ assertEquals(NOT_SUPPORTED_OPERATION, ex.getResult());
+ }
+ }
+
+ @Test
+ public void testOMLayoutVersionManagerReset() throws IOException {
Review comment:
Just curious about this scenario, when will we invoke OMLayoutVersionManager#resetLayoutVersionManager call.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] avijayanhwx commented on pull request #1393: HDDS-4143. Implement a factory for OM Requests that returns an instance based on layout version.
Posted by GitBox <gi...@apache.org>.
avijayanhwx commented on pull request #1393:
URL: https://github.com/apache/hadoop-ozone/pull/1393#issuecomment-688526180
Thanks for the review @linyiqun. I will be addressing the comments in the next PR that I create.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] avijayanhwx commented on a change in pull request #1393: HDDS-4143. Implement a factory for OM Requests that returns an instance based on layout version.
Posted by GitBox <gi...@apache.org>.
avijayanhwx commented on a change in pull request #1393:
URL: https://github.com/apache/hadoop-ozone/pull/1393#discussion_r484576560
##########
File path: hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/upgrade/TestOMVersionManager.java
##########
@@ -54,11 +61,39 @@ public void testOMLayoutVersionManager() throws IOException {
assertEquals(2, omVersionManager.getMetadataLayoutVersion());
}
+ @Test
+ public void testOMLayoutVersionManagerInitError() {
+ OMStorage omStorage = mock(OMStorage.class);
+ when(omStorage.getLayoutVersion()).thenReturn(
+ OMLayoutFeature.values()[OMLayoutFeature.values().length - 1]
+ .layoutVersion() + 1);
+ try {
+ OMLayoutVersionManager.initialize(omStorage);
+ Assert.fail();
+ } catch (OMException ex) {
+ assertEquals(NOT_SUPPORTED_OPERATION, ex.getResult());
+ }
+ }
+
+ @Test
+ public void testOMLayoutVersionManagerReset() throws IOException {
Review comment:
@linyiqun That was done just for testing purposes. In the future, we may move away from "static" Version Manager class, and that time we can remove this.
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/protocolPB/OzoneManagerRequestHandler.java
##########
@@ -290,6 +292,11 @@ public void validateRequest(OMRequest omRequest) throws OMException {
throw new OMException("ClientId is null",
OMException.ResultCodes.INVALID_REQUEST);
}
+
+ if (omRequest.getLayoutVersion() == null) {
Review comment:
Sure, will add.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] avijayanhwx closed pull request #1393: HDDS-4143. Implement a factory for OM Requests that returns an instance based on layout version.
Posted by GitBox <gi...@apache.org>.
avijayanhwx closed pull request #1393:
URL: https://github.com/apache/hadoop-ozone/pull/1393
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] avijayanhwx commented on a change in pull request #1393: HDDS-4143. Implement a factory for OM Requests that returns an instance based on layout version.
Posted by GitBox <gi...@apache.org>.
avijayanhwx commented on a change in pull request #1393:
URL: https://github.com/apache/hadoop-ozone/pull/1393#discussion_r484576613
##########
File path: hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/upgrade/TestOmRequestFactory.java
##########
@@ -0,0 +1,116 @@
+/**
+ * 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.om.upgrade;
+
+import static org.apache.hadoop.ozone.om.upgrade.OMLayoutFeature.CREATE_EC;
+import static org.apache.hadoop.ozone.om.upgrade.OMLayoutFeature.INITIAL_VERSION;
+import static org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.Type.CreateKey;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.lang.reflect.Modifier;
+import java.util.Set;
+
+import org.apache.hadoop.ozone.om.OMStorage;
+import org.apache.hadoop.ozone.om.exceptions.OMException;
+import org.apache.hadoop.ozone.om.request.OMClientRequest;
+import org.apache.hadoop.ozone.om.request.key.OMECKeyCreateRequest;
+import org.apache.hadoop.ozone.om.request.key.OMKeyCreateRequest;
+import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.LayoutVersion;
+import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.OMRequest;
+import org.junit.Assert;
+import org.junit.BeforeClass;
+import org.junit.Test;
+import org.reflections.Reflections;
+
+/**
+ * Test OmVersionFactory.
+ */
+public class TestOmRequestFactory {
+
+ private static OmRequestFactory omRequestFactory;
+ private static OMLayoutVersionManager omVersionManager;
+
+ @BeforeClass
+ public static void setup() throws OMException {
+ OMStorage omStorage = mock(OMStorage.class);
+ when(omStorage.getLayoutVersion()).thenReturn(0);
+ omVersionManager = OMLayoutVersionManager.initialize(omStorage);
+ omRequestFactory = omVersionManager.getVersionFactory();
+ }
+
+ @Test
+ public void testKeyCreateRequest() {
+
+ // Try getting v1 of 'CreateKey'.
+ Class<? extends OMClientRequest> requestType =
+ omRequestFactory.getRequestType(OMRequest.newBuilder()
+ .setCmdType(CreateKey)
+ .setClientId("c1")
+ .setLayoutVersion(LayoutVersion
+ .newBuilder()
+ .setVersion(INITIAL_VERSION.layoutVersion())
+ .build())
+ .build());
+ Assert.assertEquals(requestType, OMKeyCreateRequest.class);
+
+ // Try getting 'CreateECKey' (V2). Should fail.
+ try {
+ omRequestFactory.getRequestType(OMRequest.newBuilder()
+ .setCmdType(CreateKey)
+ .setClientId("c1")
+ .setLayoutVersion(LayoutVersion
+ .newBuilder()
+ .setVersion(CREATE_EC.layoutVersion())
+ .build())
+ .build());
+ Assert.fail();
+ } catch (IllegalArgumentException ex) {
Review comment:
Will do.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] avijayanhwx commented on pull request #1393: HDDS-4143. Implement a factory for OM Requests that returns an instance based on layout version.
Posted by GitBox <gi...@apache.org>.
avijayanhwx commented on pull request #1393:
URL: https://github.com/apache/hadoop-ozone/pull/1393#issuecomment-686922581
Marking as DRAFT until there are no major problems 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org