HDDS-9638. [hsync] File recovery support in OM #5847

ChenSammi · 2023-12-21T07:32:56Z

What changes were proposed in this pull request?

Implement file recovery support in OM.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-9638

How was this patch tested?

new unit tests and existing tests.

jojochuang · 2023-12-21T19:22:09Z

There's a compilation error that needs attention.

jojochuang

I'm half way through the review.

jojochuang · 2024-01-02T22:38:31Z

...n/java/org/apache/hadoop/ozone/om/protocolPB/OzoneManagerProtocolClientSideTranslatorPB.java

    RecoverLeaseResponse recoverLeaseResponse =
            handleError(submitRequest(omRequest)).getRecoverLeaseResponse();
-    return recoverLeaseResponse.getResponse();
+    ArrayList list = new ArrayList();


ArrayList has no type parameter

jojochuang · 2024-01-02T22:39:22Z

...n/java/org/apache/hadoop/ozone/om/protocolPB/OzoneManagerProtocolClientSideTranslatorPB.java

-    return recoverLeaseResponse.getResponse();
+    ArrayList list = new ArrayList();
+    list.add(OmKeyInfo.getFromProtobuf(recoverLeaseResponse.getKeyInfo()));
+    list.add(OmKeyInfo.getFromProtobuf(recoverLeaseResponse.getOpenKeyInfo()));


Looks like the second element is not used by callers.

@jojochuang , openKeyInfo is return for client. There is one case, suppose a hsynced file, a new block is allocated to it, then client writes some data to this new block, and crashes before it calls hsync for data on this new block, then the openKeyInfo will have one more block than keyInfo. In this case, If we want to recover the last new block length, then we need the openKeyInfo info. If we only recover the last block that hsynced ever called, then keyInfo is enough. The question is, what's expectation from user? Does recovering the last hsynced block is user's expectation?

jojochuang · 2024-01-02T22:41:19Z

hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestLeaseRecovery.java

        Thread.sleep(1000);
      }
      // The lease should have been recovered.
-      assertTrue("File should be closed", fs.recoverLease(file));


Does recoverLease() throw except if the file is already closed? If so, it would be a deviation from HDFS.

Yes, the second call of recoverLease will fail if the first has succeeded, since the file is already committed so checks in OM side will fail. Should we keep the same behavior as HDFS? I remember @szetszwo has mentioned a case that if after a Hbase region server fails, two new Hbase region servers start on the server, if the second region server calls recoverLease and gets a successful result, then there could be two region servers started and running on the same server. Not sure if Hbase has other checks to prevent the second region server to start so that the two region servers running altogether will not happen.

jojochuang · 2024-01-02T22:47:28Z

hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto

 }

 message RecoverLeaseResponse {
-  optional bool response = 1;


This is potentially incompatible. But given that the feature is disabled by default, I agree this is acceptable.

jojochuang · 2024-01-02T22:57:17Z

...p-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRequest.java

    return value != null ? value.getCacheValue() : null;
  }

+  @NotNull


This is moved from OMKeyCommitRequest.

This can move back. It's move in the first implementation edition where the key commit is called from OMRecoverLeaseRequest. Later I found the key commit in OMRecoverLeaseRequest missed to handle many thing, such as bucket quota check, bucket used bytes update, reallocated but not used block release. All these are already addressed in OMKeyCommitRequestWithFSO. So use OMKeyCommitRequestWithFSO to do the final key commit is a better way than commit in OMRecoverLeaseRequest. That's current edition.

ashishkumar50

@ChenSammi Thanks for the patch, Please find few comments inline.

ashishkumar50 · 2024-01-03T10:38:32Z

hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/OzoneClientAdapter.java

      throws IOException, InterruptedException;

-  boolean recoverLease(String pathStr) throws IOException;
+  List<OmKeyInfo> recoverFilePrepare(String pathStr) throws IOException;


Do we need to get list here? I think only openKey info may be required here.

openKeyInfo has all the allocated blocks. If the file has multiple preallocated blocks, we actually don't know how many blocks are actually used, because each block in openKeyInfo only has the allocated block length, doesn't have the real data length. And keyInfo will have real data length for each block, but keyInfo will not have preallocated by not used yet blocks info.

ashishkumar50 · 2024-01-03T10:46:11Z

hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto


 message RecoverLeaseResponse {
-  optional bool response = 1;
+  optional KeyInfo keyInfo = 1;


keyInfo is required or just openKeyInfo would suffice here?

Refer to one of above answer to Wei-Chiu's comment.

ashishkumar50 · 2024-01-03T11:15:59Z

...e/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java

-    if (isHSync) {
+    boolean isHSync = commitKeyRequest.hasHsync() && commitKeyRequest.getHsync();
+    boolean isRecovery = commitKeyRequest.hasRecovery() && commitKeyRequest.getRecovery();
+    boolean realCommit = (!isHSync) || (isHSync && isRecovery);


Can we decouple hsync and recovery. We can just use recovery flag to determine that request is for commit.

The current design is we only recover hsynced file, so that's why check both hsync and recovery flag.

I see it's changed now.

ashishkumar50 · 2024-01-03T12:15:25Z

...ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/OMRequestTestUtils.java

                      parentId);
      OMRequestTestUtils.addDirKeyToDirTable(true, omDirInfo,
-              volumeName, bucketName, txnID, omMetaMgr);
+              volumeName, bucketName, ++txnID, omMetaMgr);


Is this change(++txnID) required?

From txn ID point of view, ID should be different for every update operation. Although here in the test, previous same ID doesn't cause any problem since it's not checked in the test. But if later, some new tests start to check the ID, then they will have problems.

ashishkumar50 · 2024-01-03T12:30:53Z

...manager/src/test/java/org/apache/hadoop/ozone/om/request/file/TestOMRecoverLeaseRequest.java

+            OzoneObj.ResourceType.class)))
+        .thenReturn("user");
+    InetSocketAddress address = new InetSocketAddress("localhost", 10000);
+    when(ozoneManager.getOmRpcServerAddr()).thenReturn(address);


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.

Actually not needed, removed them.

ashishkumar50 · 2024-01-03T12:40:32Z

...one-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMRecoverLeaseRequest.java

-  private String doWork(OzoneManager ozoneManager, long transactionLogIndex)
-      throws IOException {
-
+  private RecoverLeaseResponse doWork(OzoneManager ozoneManager,


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.

Since the patch is quite big now, the support of soft and hard limit is not implemented in this patch. We can file new JIRA to do that.

Yes makes sense.

jojochuang · 2024-01-03T20:41:17Z

hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/Statistic.java

      "Calls of isFileClosed()"),
  INVOCATION_RECOVER_LEASE("op_recover_lease",
      "Calls of recoverLease()"),
+  INVOCATION_COMMIT("op_commit", "Calls of commit()"),


should it be called INVOCATION_RECOVER_FILE instead?

...-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequestWithFSO.java

ashishkumar50 · 2024-01-08T15:16:40Z

@ChenSammi, Thanks for updating patch, overall patch LGTM.

jojochuang

+1 let's merge and proceed.

smengcl · 2024-01-22T02:38:53Z

...one-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMRecoverLeaseRequest.java

+      // Add to cache.
+      omMetadataManager.getOpenKeyTable(getBucketLayout()).addCacheEntry(
+          new CacheKey<>(dbOpenFileKey), CacheValue.get(transactionLogIndex, openKeyInfo));


nit: we could use the cleaner form instead now that @szetszwo added it in HDDS-7969:

bd900ae#diff-ac3f28f30c26932cfbaf4270f5f0862b973b4e3e5b9b11ae78e725c306736fcdL324-L326

i.e.

Suggested change

// Add to cache.

omMetadataManager.getOpenKeyTable(getBucketLayout()).addCacheEntry(

new CacheKey<>(dbOpenFileKey), CacheValue.get(transactionLogIndex, openKeyInfo));

// Add to cache.

omMetadataManager.getOpenKeyTable(getBucketLayout()).addCacheEntry(

dbOpenFileKey, openKeyInfo, transactionLogIndex);

ChenSammi force-pushed the HDDS-9638 branch 2 times, most recently from fffceae to f474013 Compare December 21, 2023 09:32

HDDS-9638. [hsync] File recovery support in OM

dc9ccb5

ChenSammi force-pushed the HDDS-9638 branch from 216936f to dc9ccb5 Compare December 27, 2023 08:25

jojochuang reviewed Jan 3, 2024

View reviewed changes

ashishkumar50 reviewed Jan 3, 2024

View reviewed changes

jojochuang reviewed Jan 4, 2024

View reviewed changes

ChenSammi added 3 commits January 5, 2024 11:22

address comments

fd11328

remove unused import

6408e2e

fix failed UT

a290f45

jojochuang approved these changes Jan 9, 2024

View reviewed changes

jojochuang merged commit c45449c into apache:HDDS-7593 Jan 9, 2024

jojochuang pushed a commit to jojochuang/ozone that referenced this pull request Jan 17, 2024

HDDS-9638. [hsync] File recovery support in OM (apache#5847)

3d872e3

jojochuang pushed a commit to jojochuang/ozone that referenced this pull request Jan 17, 2024

HDDS-9638. [hsync] File recovery support in OM (apache#5847)

5a42f3d

jojochuang pushed a commit to jojochuang/ozone that referenced this pull request Jan 17, 2024

HDDS-9638. [hsync] File recovery support in OM (apache#5847)

b16087a

smengcl reviewed Jan 22, 2024

View reviewed changes

jojochuang added the hbase HBase on Ozone support label Jan 23, 2024

smengcl mentioned this pull request Jan 29, 2024

HDDS-10077. Add hsync metadata to hsync'ed keys in OpenKeyTable as well #6046

Merged

2 tasks

chungen0126 pushed a commit to chungen0126/ozone that referenced this pull request May 3, 2024

HDDS-9638. [hsync] File recovery support in OM (apache#5847)

6493fa7

chungen0126 pushed a commit to chungen0126/ozone that referenced this pull request May 3, 2024

HDDS-9638. [hsync] File recovery support in OM (apache#5847)

58dbe33

HDDS-9638. [hsync] File recovery support in OM #5847

HDDS-9638. [hsync] File recovery support in OM #5847

Uh oh!

Conversation

ChenSammi commented Dec 21, 2023

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

jojochuang commented Dec 21, 2023

Uh oh!

jojochuang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChenSammi Jan 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ashishkumar50 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ashishkumar50 commented Jan 8, 2024

Uh oh!

jojochuang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smengcl Jan 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

ChenSammi Jan 5, 2024 •

edited

Loading

smengcl Jan 22, 2024 •

edited

Loading