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/01/31 18:49:57 UTC
[GitHub] [hadoop-ozone] hanishakoneru opened a new pull request #513:
HDDS-2962. Handle replay of OM Prefix ACL requests
hanishakoneru opened a new pull request #513: HDDS-2962. Handle replay of OM Prefix ACL requests
URL: https://github.com/apache/hadoop-ozone/pull/513
## What changes were proposed in this pull request?
To ensure that Prefix acl operations are idempotent, compare the transactionID with the objectID and updateID to make sure that the transaction is not a replay. If the transactionID <= updateID, then it implies that the transaction is a replay and hence it should be skipped.
OMPrefixAclRequests (Add, Remove and Set ACL requests) are made idempotent in this Jira.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-2962
## How was this patch tested?
Unit tests added.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] arp7 commented on a change in pull request #513:
HDDS-2962. Handle replay of OM Prefix ACL requests
Posted by GitBox <gi...@apache.org>.
arp7 commented on a change in pull request #513: HDDS-2962. Handle replay of OM Prefix ACL requests
URL: https://github.com/apache/hadoop-ozone/pull/513#discussion_r374265219
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/PrefixManagerImpl.java
##########
@@ -176,9 +177,10 @@ public boolean setAcl(OzoneObj obj, List<OzoneAcl> acls) throws IOException {
OmPrefixInfo prefixInfo =
metadataManager.getPrefixTable().get(prefixPath);
- OMPrefixAclOpResult omPrefixAclOpResult = setAcl(obj, acls, prefixInfo);
+ OMPrefixAclOpResult omPrefixAclOpResult = setAcl(obj, acls, prefixInfo,
+ 0L);
Review comment:
Dumb q. - why pass 0 here?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] arp7 commented on a change in pull request #513:
HDDS-2962. Handle replay of OM Prefix ACL requests
Posted by GitBox <gi...@apache.org>.
arp7 commented on a change in pull request #513: HDDS-2962. Handle replay of OM Prefix ACL requests
URL: https://github.com/apache/hadoop-ozone/pull/513#discussion_r374276988
##########
File path: hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMPrefixAclRequest.java
##########
@@ -0,0 +1,101 @@
+/**
+ * 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.request.key;
+
+import java.util.UUID;
+import org.apache.hadoop.ozone.OzoneAcl;
+import org.apache.hadoop.ozone.om.PrefixManager;
+import org.apache.hadoop.ozone.om.PrefixManagerImpl;
+import org.apache.hadoop.ozone.om.request.TestOMRequestUtils;
+import org.apache.hadoop.ozone.om.request.key.acl.prefix.OMPrefixAddAclRequest;
+import org.apache.hadoop.ozone.om.response.OMClientResponse;
+import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos;
+import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.AddAclRequest;
+import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.OMRequest;
+import org.apache.hadoop.ozone.security.acl.OzoneObj;
+import org.apache.hadoop.ozone.security.acl.OzoneObjInfo;
+import org.junit.Assert;
+import org.junit.Test;
+
+import static org.mockito.Mockito.when;
+
+/**
+ * Tests Prefix ACL requests.
+ */
+public class TestOMPrefixAclRequest extends TestOMKeyRequest {
+
+ @Test
+ public void testReplayRequest() throws Exception {
Review comment:
Yes we must have tests for proto [de]serialization. However I am not sure we have been rigorous about adding them in the past.
Let's file a separate newbie jira to add missing protobuf [de]serialization unit tests.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] arp7 commented on a change in pull request #513:
HDDS-2962. Handle replay of OM Prefix ACL requests
Posted by GitBox <gi...@apache.org>.
arp7 commented on a change in pull request #513: HDDS-2962. Handle replay of OM Prefix ACL requests
URL: https://github.com/apache/hadoop-ozone/pull/513#discussion_r374273789
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/PrefixManagerImpl.java
##########
@@ -176,9 +177,10 @@ public boolean setAcl(OzoneObj obj, List<OzoneAcl> acls) throws IOException {
OmPrefixInfo prefixInfo =
metadataManager.getPrefixTable().get(prefixPath);
- OMPrefixAclOpResult omPrefixAclOpResult = setAcl(obj, acls, prefixInfo);
+ OMPrefixAclOpResult omPrefixAclOpResult = setAcl(obj, acls, prefixInfo,
+ 0L);
Review comment:
You're right.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] hanishakoneru commented on a change in pull request
#513: HDDS-2962. Handle replay of OM Prefix ACL requests
Posted by GitBox <gi...@apache.org>.
hanishakoneru commented on a change in pull request #513: HDDS-2962. Handle replay of OM Prefix ACL requests
URL: https://github.com/apache/hadoop-ozone/pull/513#discussion_r374270813
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/PrefixManagerImpl.java
##########
@@ -176,9 +177,10 @@ public boolean setAcl(OzoneObj obj, List<OzoneAcl> acls) throws IOException {
OmPrefixInfo prefixInfo =
metadataManager.getPrefixTable().get(prefixPath);
- OMPrefixAclOpResult omPrefixAclOpResult = setAcl(obj, acls, prefixInfo);
+ OMPrefixAclOpResult omPrefixAclOpResult = setAcl(obj, acls, prefixInfo,
+ 0L);
Review comment:
This code path was used for the non-ratis case before the two code paths were merged. This should go away after code cleanup.
@bharatviswa504 please correct me if I am wrong.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] hanishakoneru commented on a change in pull request
#513: HDDS-2962. Handle replay of OM Prefix ACL requests
Posted by GitBox <gi...@apache.org>.
hanishakoneru commented on a change in pull request #513: HDDS-2962. Handle replay of OM Prefix ACL requests
URL: https://github.com/apache/hadoop-ozone/pull/513#discussion_r374274844
##########
File path: hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMPrefixAclRequest.java
##########
@@ -0,0 +1,101 @@
+/**
+ * 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.request.key;
+
+import java.util.UUID;
+import org.apache.hadoop.ozone.OzoneAcl;
+import org.apache.hadoop.ozone.om.PrefixManager;
+import org.apache.hadoop.ozone.om.PrefixManagerImpl;
+import org.apache.hadoop.ozone.om.request.TestOMRequestUtils;
+import org.apache.hadoop.ozone.om.request.key.acl.prefix.OMPrefixAddAclRequest;
+import org.apache.hadoop.ozone.om.response.OMClientResponse;
+import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos;
+import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.AddAclRequest;
+import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.OMRequest;
+import org.apache.hadoop.ozone.security.acl.OzoneObj;
+import org.apache.hadoop.ozone.security.acl.OzoneObjInfo;
+import org.junit.Assert;
+import org.junit.Test;
+
+import static org.mockito.Mockito.when;
+
+/**
+ * Tests Prefix ACL requests.
+ */
+public class TestOMPrefixAclRequest extends TestOMKeyRequest {
+
+ @Test
+ public void testReplayRequest() throws Exception {
Review comment:
I think it should be. This test case does not cover the change in logging on completion. But all the PrefixAcl requests go through a common code path for replay check which is covered here.
Do we want to add a test for proto changes?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] arp7 commented on a change in pull request #513:
HDDS-2962. Handle replay of OM Prefix ACL requests
Posted by GitBox <gi...@apache.org>.
arp7 commented on a change in pull request #513: HDDS-2962. Handle replay of OM Prefix ACL requests
URL: https://github.com/apache/hadoop-ozone/pull/513#discussion_r374270565
##########
File path: hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMPrefixAclRequest.java
##########
@@ -0,0 +1,101 @@
+/**
+ * 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.request.key;
+
+import java.util.UUID;
+import org.apache.hadoop.ozone.OzoneAcl;
+import org.apache.hadoop.ozone.om.PrefixManager;
+import org.apache.hadoop.ozone.om.PrefixManagerImpl;
+import org.apache.hadoop.ozone.om.request.TestOMRequestUtils;
+import org.apache.hadoop.ozone.om.request.key.acl.prefix.OMPrefixAddAclRequest;
+import org.apache.hadoop.ozone.om.response.OMClientResponse;
+import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos;
+import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.AddAclRequest;
+import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.OMRequest;
+import org.apache.hadoop.ozone.security.acl.OzoneObj;
+import org.apache.hadoop.ozone.security.acl.OzoneObjInfo;
+import org.junit.Assert;
+import org.junit.Test;
+
+import static org.mockito.Mockito.when;
+
+/**
+ * Tests Prefix ACL requests.
+ */
+public class TestOMPrefixAclRequest extends TestOMKeyRequest {
+
+ @Test
+ public void testReplayRequest() throws Exception {
Review comment:
Is one unit test case sufficient? Are we covering all the new code paths?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] arp7 merged pull request #513: HDDS-2962. Handle
replay of OM Prefix ACL requests
Posted by GitBox <gi...@apache.org>.
arp7 merged pull request #513: HDDS-2962. Handle replay of OM Prefix ACL requests
URL: https://github.com/apache/hadoop-ozone/pull/513
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org