HDDS-9638. [hsync] File recovery support in OM #5847
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -149,8 +149,7 @@ public void testRecovery() throws Exception { | |
| Thread.sleep(1000); | ||
| } | ||
| // The lease should have been recovered. | ||
|
Contributor
There was a problem hiding this comment. Does recoverLease() throw except if the file is already closed? If so, it would be a deviation from HDFS.
Contributor
Author
There was a problem hiding this comment. 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. |
||
| assertTrue("File should be closed", fs.isFileClosed(file)); | ||
| } finally { | ||
| closeIgnoringKeyNotFound(stream); | ||
| } | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -20,6 +20,8 @@ | |||||||||||||
|
|
||||||||||||||
| import com.google.common.base.Preconditions; | ||||||||||||||
|
|
||||||||||||||
| import org.apache.hadoop.hdds.utils.db.cache.CacheKey; | ||||||||||||||
| import org.apache.hadoop.hdds.utils.db.cache.CacheValue; | ||||||||||||||
| import org.apache.hadoop.ozone.OzoneConsts; | ||||||||||||||
| import org.apache.hadoop.ozone.audit.OMAction; | ||||||||||||||
| import org.apache.hadoop.ozone.om.OMMetadataManager; | ||||||||||||||
|
|
@@ -34,14 +36,11 @@ | |||||||||||||
| import org.apache.hadoop.ozone.om.request.util.OmResponseUtil; | ||||||||||||||
| import org.apache.hadoop.ozone.om.response.OMClientResponse; | ||||||||||||||
| import org.apache.hadoop.ozone.om.response.file.OMRecoverLeaseResponse; | ||||||||||||||
| import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.OMResponse; | ||||||||||||||
| import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.OMRequest; | ||||||||||||||
| import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.RecoverLeaseRequest; | ||||||||||||||
| import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.RecoverLeaseResponse; | ||||||||||||||
| import static org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.Type.RecoverLease; | ||||||||||||||
|
|
||||||||||||||
| import org.apache.hadoop.ozone.security.acl.IAccessAuthorizer; | ||||||||||||||
| import org.apache.hadoop.ozone.security.acl.OzoneObj; | ||||||||||||||
|
|
@@ -58,10 +57,9 @@ | |||||||||||||
| import java.util.LinkedHashMap; | ||||||||||||||
| import java.util.Map; | ||||||||||||||
|
|
||||||||||||||
| import static org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes.KEY_ALREADY_CLOSED; | ||||||||||||||
| import static org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes.KEY_NOT_FOUND; | ||||||||||||||
| import static org.apache.hadoop.ozone.om.lock.OzoneManagerLock.Resource.BUCKET_LOCK; | ||||||||||||||
|
|
||||||||||||||
| /** | ||||||||||||||
| * Perform actions for RecoverLease requests. | ||||||||||||||
|
|
@@ -75,6 +73,8 @@ public class OMRecoverLeaseRequest extends OMKeyRequest { | |||||||||||||
| private String keyName; | ||||||||||||||
| private OmKeyInfo keyInfo; | ||||||||||||||
| private String dbFileKey; | ||||||||||||||
| private OmKeyInfo openKeyInfo; | ||||||||||||||
| private String dbOpenFileKey; | ||||||||||||||
|
|
||||||||||||||
| private OMMetadataManager omMetadataManager; | ||||||||||||||
|
|
||||||||||||||
|
|
@@ -141,28 +141,21 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, | |||||||||||||
| acquiredLock = getOmLockDetails().isLockAcquired(); | ||||||||||||||
| validateBucketAndVolume(omMetadataManager, volumeName, bucketName); | ||||||||||||||
|
|
||||||||||||||
| RecoverLeaseResponse recoverLeaseResponse = doWork(ozoneManager, transactionLogIndex); | ||||||||||||||
|
|
||||||||||||||
| // Prepare response | ||||||||||||||
| boolean responseCode = true; | ||||||||||||||
| omResponse.setRecoverLeaseResponse(recoverLeaseResponse).setCmdType(RecoverLease); | ||||||||||||||
| omClientResponse = new OMRecoverLeaseResponse(omResponse.build(), getBucketLayout(), | ||||||||||||||
| dbOpenFileKey, openKeyInfo); | ||||||||||||||
| omMetrics.incNumRecoverLease(); | ||||||||||||||
| LOG.debug("Key recovered. Volume:{}, Bucket:{}, Key:{}", | ||||||||||||||
| volumeName, bucketName, keyName); | ||||||||||||||
| } catch (IOException | InvalidPathException ex) { | ||||||||||||||
| LOG.error("Fail for recovering lease. Volume:{}, Bucket:{}, Key:{}", | ||||||||||||||
| volumeName, bucketName, keyName, ex); | ||||||||||||||
| exception = ex; | ||||||||||||||
| omMetrics.incNumRecoverLeaseFails(); | ||||||||||||||
| omClientResponse = new OMRecoverLeaseResponse( | ||||||||||||||
| createErrorOMResponse(omResponse, exception), getBucketLayout()); | ||||||||||||||
| } finally { | ||||||||||||||
|
|
@@ -186,60 +179,56 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, | |||||||||||||
| return omClientResponse; | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| private RecoverLeaseResponse doWork(OzoneManager ozoneManager, | ||||||||||||||
|
Contributor
There was a problem hiding this 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.
Contributor
Author
There was a problem hiding this comment. 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. |
||||||||||||||
| long transactionLogIndex) throws IOException { | ||||||||||||||
| final long volumeId = omMetadataManager.getVolumeId(volumeName); | ||||||||||||||
| final long bucketId = omMetadataManager.getBucketId(volumeName, bucketName); | ||||||||||||||
| Iterator<Path> pathComponents = Paths.get(keyName).iterator(); | ||||||||||||||
| long parentID = OMFileRequest.getParentID(volumeId, bucketId, | ||||||||||||||
| pathComponents, keyName, omMetadataManager, | ||||||||||||||
| "Cannot recover file : " + keyName | ||||||||||||||
| + " as parent directory doesn't exist"); | ||||||||||||||
| String fileName = OzoneFSUtils.getFileName(keyName); | ||||||||||||||
| dbFileKey = omMetadataManager.getOzonePathKey(volumeId, bucketId, parentID, fileName); | ||||||||||||||
|
|
||||||||||||||
| keyInfo = getKey(dbFileKey); | ||||||||||||||
| if (keyInfo == null) { | ||||||||||||||
| throw new OMException("Key:" + keyName + " not found in keyTable", KEY_NOT_FOUND); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| final String writerId = keyInfo.getMetadata().get(OzoneConsts.HSYNC_CLIENT_ID); | ||||||||||||||
| if (writerId == null) { | ||||||||||||||
| // if file is closed, do nothing and return right away. | ||||||||||||||
| throw new OMException("Key: " + keyName + " is already closed", KEY_ALREADY_CLOSED); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| dbOpenFileKey = omMetadataManager.getOpenFileName( | ||||||||||||||
| volumeId, bucketId, parentID, fileName, Long.parseLong(writerId)); | ||||||||||||||
| openKeyInfo = omMetadataManager.getOpenKeyTable(getBucketLayout()).get(dbOpenFileKey); | ||||||||||||||
| if (openKeyInfo == null) { | ||||||||||||||
| throw new OMException("Open Key " + dbOpenFileKey + " not found in openKeyTable", KEY_NOT_FOUND); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| if (openKeyInfo.getMetadata().containsKey(OzoneConsts.LEASE_RECOVERY)) { | ||||||||||||||
| LOG.debug("Key: " + keyName + " is already under recovery"); | ||||||||||||||
| } else { | ||||||||||||||
| openKeyInfo.getMetadata().put(OzoneConsts.LEASE_RECOVERY, "true"); | ||||||||||||||
| openKeyInfo.setUpdateID(transactionLogIndex, ozoneManager.isRatisEnabled()); | ||||||||||||||
| openKeyInfo.setModificationTime(Time.now()); | ||||||||||||||
| // Add to cache. | ||||||||||||||
| omMetadataManager.getOpenKeyTable(getBucketLayout()).addCacheEntry( | ||||||||||||||
| new CacheKey<>(dbOpenFileKey), CacheValue.get(transactionLogIndex, openKeyInfo)); | ||||||||||||||
|
Comment on lines
+218
to
+220
Contributor
There was a problem hiding this comment. nit: we could use the cleaner form instead now that @szetszwo added it in HDDS-7969: bd900ae#diff-ac3f28f30c26932cfbaf4270f5f0862b973b4e3e5b9b11ae78e725c306736fcdL324-L326
Contributor
There was a problem hiding this comment. i.e.
Suggested change
|
||||||||||||||
| } | ||||||||||||||
| keyInfo.setKeyName(keyName); | ||||||||||||||
| openKeyInfo.setKeyName(keyName); | ||||||||||||||
| RecoverLeaseResponse.Builder rb = RecoverLeaseResponse.newBuilder() | ||||||||||||||
| .setOpenKeyInfo(openKeyInfo.getNetworkProtobuf(getOmRequest().getVersion(), true)) | ||||||||||||||
| .setKeyInfo(keyInfo.getNetworkProtobuf(getOmRequest().getVersion(), true)); | ||||||||||||||
|
|
||||||||||||||
| return rb.build(); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| private OmKeyInfo getKey(String dbOzoneKey) throws IOException { | ||||||||||||||
| return omMetadataManager.getKeyTable(getBucketLayout()).get(dbOzoneKey); | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the second element is not used by callers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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?