ashishkumar50 commented on code in PR #5847:
URL: https://github.com/apache/ozone/pull/5847#discussion_r1440303590
##########
hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/OzoneClientAdapter.java:
##########
@@ -95,7 +97,9 @@ SnapshotDiffReport getSnapshotDiffReport(Path snapshotDir,
String fromSnapshot, String toSnapshot)
throws IOException, InterruptedException;
- boolean recoverLease(String pathStr) throws IOException;
+ List<OmKeyInfo> recoverFilePrepare(String pathStr) throws IOException;
Review Comment:
Do we need to get list here? I think only openKey info may be required here.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java:
##########
@@ -156,17 +160,17 @@ public OMClientResponse
validateAndUpdateCache(OzoneManager ozoneManager,
OMMetadataManager omMetadataManager = ozoneManager.getMetadataManager();
- boolean isHSync = commitKeyRequest.hasHsync() &&
- commitKeyRequest.getHsync();
-
- if (isHSync) {
+ boolean isHSync = commitKeyRequest.hasHsync() &&
commitKeyRequest.getHsync();
+ boolean isRecovery = commitKeyRequest.hasRecovery() &&
commitKeyRequest.getRecovery();
+ boolean realCommit = (!isHSync) || (isHSync && isRecovery);
Review Comment:
Can we decouple hsync and recovery. We can just use recovery flag to
determine that request is for commit.
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/file/TestOMRecoverLeaseRequest.java:
##########
@@ -79,26 +91,230 @@ public void testRecoverHsyncFile() throws Exception {
.thenReturn("user");
InetSocketAddress address = new InetSocketAddress("localhost", 10000);
when(ozoneManager.getOmRpcServerAddr()).thenReturn(address);
- populateNamespace(true, true);
+ populateNamespace(true, true, true, true);
OMClientResponse omClientResponse = validateAndUpdateCache();
+ OMResponse omResponse = omClientResponse.getOMResponse();
+ Assertions.assertEquals(OzoneManagerProtocolProtos.Status.OK,
omResponse.getStatus());
+ RecoverLeaseResponse recoverLeaseResponse =
omResponse.getRecoverLeaseResponse();
+ KeyInfo keyInfo = recoverLeaseResponse.getKeyInfo();
+ Assertions.assertNotNull(keyInfo);
+ OmKeyInfo omKeyInfo = OmKeyInfo.getFromProtobuf(keyInfo);
- Assertions.assertEquals(OzoneManagerProtocolProtos.Status.OK,
+ omClientResponse =
validateAndUpdateCacheForCommit(getNewKeyArgs(omKeyInfo, 0));
+ omResponse = omClientResponse.getOMResponse();
+ Assertions.assertEquals(OzoneManagerProtocolProtos.Status.OK,
omResponse.getStatus());
+
+ verifyTables(true, false);
+ }
+
+ /**
+ * Verify that RecoverLease request is idempotent.
+ * @throws Exception
+ */
+ @Test
+ public void testInitStageIdempotent() throws Exception {
+ when(ozoneManager.getAclsEnabled()).thenReturn(true);
+ when(ozoneManager.getVolumeOwner(
+ anyString(),
+ any(IAccessAuthorizer.ACLType.class), any(
+ OzoneObj.ResourceType.class)))
+ .thenReturn("user");
+ InetSocketAddress address = new InetSocketAddress("localhost", 10000);
+ when(ozoneManager.getOmRpcServerAddr()).thenReturn(address);
+ populateNamespace(true, true, true, true);
+
+ // call recovery first time
+ OMClientResponse omClientResponse = validateAndUpdateCache();
+ OMResponse omResponse = omClientResponse.getOMResponse();
+ Assertions.assertEquals(OzoneManagerProtocolProtos.Status.OK,
omResponse.getStatus());
+ RecoverLeaseResponse recoverLeaseResponse =
omResponse.getRecoverLeaseResponse();
+ KeyInfo keyInfo1 = recoverLeaseResponse.getKeyInfo();
+ Assertions.assertNotNull(keyInfo1);
+
+ // call recovery second time
+ omClientResponse = validateAndUpdateCache();
+ omResponse = omClientResponse.getOMResponse();
+ Assertions.assertEquals(OzoneManagerProtocolProtos.Status.OK,
omResponse.getStatus());
+ recoverLeaseResponse = omResponse.getRecoverLeaseResponse();
+ KeyInfo keyInfo2 = recoverLeaseResponse.getKeyInfo();
+ Assertions.assertNotNull(keyInfo2);
+ Assertions.assertEquals(keyInfo1.getKeyName(), keyInfo2.getKeyName());
+ }
+
+ /**
+ * Verify that COMMIT request for recovery is not idempotent.
+ * @throws Exception
+ */
+ @Test
+ public void testCommitStageNotIdempotent() throws Exception {
+ when(ozoneManager.getAclsEnabled()).thenReturn(true);
+ when(ozoneManager.getVolumeOwner(
+ anyString(),
+ any(IAccessAuthorizer.ACLType.class), any(
+ OzoneObj.ResourceType.class)))
+ .thenReturn("user");
+ InetSocketAddress address = new InetSocketAddress("localhost", 10000);
+ when(ozoneManager.getOmRpcServerAddr()).thenReturn(address);
+ populateNamespace(true, true, true, true);
+
+ // call recovery
+ OMClientResponse omClientResponse = validateAndUpdateCache();
+ OMResponse omResponse = omClientResponse.getOMResponse();
+ Assertions.assertEquals(OzoneManagerProtocolProtos.Status.OK,
omResponse.getStatus());
+ RecoverLeaseResponse recoverLeaseResponse =
omResponse.getRecoverLeaseResponse();
+ KeyInfo keyInfo = recoverLeaseResponse.getKeyInfo();
+ Assertions.assertNotNull(keyInfo);
+ OmKeyInfo omKeyInfo = OmKeyInfo.getFromProtobuf(keyInfo);
+
+ KeyArgs newKeyArgs = getNewKeyArgs(omKeyInfo, 0);
+
+ // call commit first time
+ omClientResponse = validateAndUpdateCacheForCommit(newKeyArgs);
+ omResponse = omClientResponse.getOMResponse();
+ Assertions.assertEquals(OzoneManagerProtocolProtos.Status.OK,
omResponse.getStatus());
+
+ // call commit second time
+ omClientResponse = validateAndUpdateCacheForCommit(newKeyArgs);
+ omResponse = omClientResponse.getOMResponse();
+
Assertions.assertEquals(OzoneManagerProtocolProtos.Status.KEY_ALREADY_CLOSED,
omResponse.getStatus());
+ }
+
+ /**
+ * Verify that RecoverLease COMMIT request has a new file length.
+ * @throws Exception
+ */
+ @Test
+ public void testRecoverWithNewFileLength() throws Exception {
+ when(ozoneManager.getAclsEnabled()).thenReturn(true);
+ when(ozoneManager.getVolumeOwner(
+ anyString(),
+ any(IAccessAuthorizer.ACLType.class), any(
+ OzoneObj.ResourceType.class)))
+ .thenReturn("user");
+ InetSocketAddress address = new InetSocketAddress("localhost", 10000);
+ when(ozoneManager.getOmRpcServerAddr()).thenReturn(address);
Review Comment:
Above ACL and address usage seems to be in every new test method, may be we
can move to init or other method and use.
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/OMRequestTestUtils.java:
##########
@@ -1571,7 +1571,7 @@ public static long addParentsToDirTable(String
volumeName, String bucketName,
OMRequestTestUtils.createOmDirectoryInfo(pathElement, ++objectId,
parentId);
OMRequestTestUtils.addDirKeyToDirTable(true, omDirInfo,
- volumeName, bucketName, txnID, omMetaMgr);
+ volumeName, bucketName, ++txnID, omMetaMgr);
Review Comment:
Is this change(`++txnID`) required?
##########
hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto:
##########
@@ -2070,7 +2073,8 @@ message RecoverLeaseRequest {
}
message RecoverLeaseResponse {
- optional bool response = 1;
+ optional KeyInfo keyInfo = 1;
Review Comment:
keyInfo is required or just openKeyInfo would suffice here?
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMRecoverLeaseRequest.java:
##########
@@ -186,60 +179,56 @@ public OMClientResponse
validateAndUpdateCache(OzoneManager ozoneManager,
return omClientResponse;
}
- private String doWork(OzoneManager ozoneManager, long transactionLogIndex)
- throws IOException {
-
+ private RecoverLeaseResponse doWork(OzoneManager ozoneManager,
Review Comment:
I think better to have restriction on some time gap between recoverLease and
latest hsync call. Or else here recoverLease can be called immediately after
hsync 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.
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]
