HDDS-10044. [hsync] File recovery support in Client #5978
Conversation
ChenSammi
force-pushed
the
HDDS-10044
branch
2 times, most recently
from
9e0b98e to
087ff8d
Compare
| // TODO: query DN to get the final block length | ||
|
|
||
| OmKeyInfo keyInfo = infoList.get(0); | ||
| // finalize the final block and get block length |
There was a problem hiding this comment.
Looks like we can extract this method so it can be reused by OzoneFileSystem.recoverLease()
|
|
||
| @Override | ||
| public long finalizeBlock(OmKeyLocationInfo block) throws IOException { | ||
| incrementCounter(Statistic.INVOCATION_FINALIZE_BLOCK, 1); |
There was a problem hiding this comment.
this method appears exactly the same as BasicOzoneClientAdapterImpl.fiinalizeBlock().
There was a problem hiding this comment.
Yes, most functions' implementation in BasicOzoneClientAdapterImpl and BasicRootedOzoneClientAdapterImpl are the same, one for BasicOzoneFileSystem, one for BasicRootedOzoneFileSystem.
There was a problem hiding this comment.
There is chance that the whole BasicOzoneClientAdapterImpl and BasicRootedOzoneClientAdapterImpl can be refactored to remove the duplicated codes.
| if (recoverLeaseResponse.hasKeyInfo()) { | ||
| list.add(OmKeyInfo.getFromProtobuf(recoverLeaseResponse.getKeyInfo())); | ||
| } else if (recoverLeaseResponse.hasOpenKeyInfo()) { | ||
| list.add(OmKeyInfo.getFromProtobuf(recoverLeaseResponse.getOpenKeyInfo())); |
There was a problem hiding this comment.
Instead of list we can just use OmkeyInfo, as in caller we are using get(0).
Also in caller we may not able to distinguish whether returned keyInfo is from openKey or Key table. Instead of list can we add a class containing openkey/key info. So that this ambiguity will not arise in future.
| Pipeline.Builder builder = Pipeline.newBuilder().setReplicationConfig(newConfig).setId(PipelineID.randomId()) | ||
| .setNodes(block.getPipeline().getNodes()).setState(Pipeline.PipelineState.OPEN); | ||
| try { | ||
| client = xceiverClientFactory.acquireClient(builder.build()); |
There was a problem hiding this comment.
Required to use acquireClientForReadData instead of acquireClient ?
There was a problem hiding this comment.
Right, acquireClientForReadData is better.
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/storage/ContainerProtocolCalls.java
Show resolved
Hide resolved
| FileStatus fileStatus = fs.getFileStatus(file); | ||
| assertEquals(dataSize, fileStatus.getLen()); | ||
| // make sure the writer can not write again. | ||
| // TODO: write does not fail here. Looks like a bug. HDDS-8439 to fix it. |
| return null; | ||
| } | ||
|
|
||
| @Nullable |
| } catch (Throwable e) { | ||
| } | ||
| cluster.getOzoneManager().restart(); | ||
| cluster.waitForClusterToBeReady(); |
There was a problem hiding this comment.
Can we verify after OM restart recovery works fine.
| OzoneTestUtils.closeContainer(scm, container); | ||
| GenericTestUtils.waitFor(() -> { | ||
| try { | ||
| return scm.getPipelineManager().getPipeline(container.getPipelineID()).isClosed(); |
There was a problem hiding this comment.
Do we need to wait here to check for pipeline is CLOSED or not? I see in other test cases we are not checking it. closeContainer() waits for container to go into CLOSED state.
There was a problem hiding this comment.
Container closed before pipeline closed. So there is chance pipeline is still OPEN when container is closed. The explicitly check here is to make sure the pipeline is closed too.
| return BlockData.getFromProtoBuf(finalizeBlockResponseProto.getBlockData()).getSize(); | ||
| } | ||
| } catch (IOException e) { | ||
| LOG.warn("Failed to execute finalizeBlock command", e); |
There was a problem hiding this comment.
Currently for all exception we are proceeding to get block length from DN. There may be case when container is still not CLOSED.
I think we should get block length from DN when container replica is CLOSED.
There was a problem hiding this comment.
Checking CLOSED state replica is my initial idea too. Then later I found there is more concise way. We can leverage this getCommittedBlockLength call regardless of the replica state given that ratis is used to update the replica data.
The implementation of getCommittedBlockLength compares the bcsid of involved block and replica's bcsid. If replica's bcsid is no less than block's bcsid, then the block info in this replica is a consensus result of raft, it's trustworthy.
|
Thanks @jojochuang and @ashishkumar50 for the review. |
3 participants
What changes were proposed in this pull request?
implement the client side function of file lease recovery.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-10044
How was this patch tested?
new unit tests