HDDS-4954. Add replicaIndex to the RPC protocols

[elek]

What changes were proposed in this pull request?

This is the initial patch to support differentiating between multiple container replicas.

With RATIS/Closed containers all the container replicas are exactly the same but with EC they will be different. We need an additional index for each container replicas to know how it should be handled.

This patch is part of bigger effort to show end2end solution for container indexes. The full picture can be found at https://github.com/elek/ozone/tree/ec with all of these improvements:

• replicaIndexes are introduced in proto files and Pipeline
• EC repliation type is added
• SCM allocates datanodes for new EC pipelines (hard coded 1 as of now)
• Client sends the instanceIndex during chunkWrites (required for container creation)
• Container files on datanodes persist the instance index
• ContainerReports contains the instanceIndex
• SCM adds the new field the ContainerReplica instances
• RelicationManager is extended to handle EC type containers (with a stub implementation)

This doesn't include all of these modifications, it is just the very first, separated step where the proto files and Pipeline are modified.

What is the link to the Apache JIRA

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

How was this patch tested?

The patch is tested together with the full end2end code (https://github.com/elek/ozone/tree/ec)

Started Ozone cluster and created a key:

./ozone sh key put -t EC /vol1/bucket1/key1 ../README.md

Checked the container yaml file if it contains the instandeIndex.

Checked the SCM log for the message from ReplicationMananger:

2021-03-17 12:30:02,754 INFO  container.ReplicationManager (ReplicationManager.java:processContainer(329)) - TODO: replication management for EC containers are missing. Container #1, 127.0.0.1 #7

[umamaheswararao]

Thanks for separating the replication type changes into separate JIRA.
Please add the replication index to org.apache.hadoop.hdds.client.BlockID and org.apache.hadoop.hdds.client.ContainerBlockID
This will be needed when we create container from client, we anyway pass blockID to DN. If index available there, then DN know which index.
Current patch seems t be covering the protocols only from SCM and DN. The above change can help to cover client needed changes.

We also discussed yesterday that, let's not make index as the property to chunkInfo. I am more comfortable if we make that as property to ContainerBlockID instead. Thanks for removing them in latest patch.

[umamaheswararao]

containerinstanceIndex -> containerInstanceIndex

[elek]

Fixed with moving this field to the DatanodeBlockID

[umamaheswararao]

I understand you may be correcting the formatting issues like organizing imports and other formatting in existing code as well. Could you please check if it's possible to keep only changed lines to be formatted? That way branch code will have less changes and we can avoid conflict issues.

[umamaheswararao]

Could you please add a comment that, returning 0 means, it's just an invalid index and this API for EC. Because mostly we may not use this kind of API in non-EC flow, we may invoke this API in EC flow to get a particular DN index.

[elek]

Sure, I added a detailed javadoc.

[elek]

We also discussed yesterday that, let's not make index as the property to chunkInfo. I am more comfortable if we make that as property to ContainerBlockID instead. Thanks for removing them in latest patch.

Sure, I removed it, but during my git rebase it's not fully moved (I manage 10-12 patches today and trying to sync them one-by-one with different PRs)

Now it should be good:

/**
 * Block ID that uniquely identify a block in Datanode.
 */
message DatanodeBlockID {
  required int64 containerID = 1;
  required int64 localID = 2;
  optional uint64 blockCommitSequenceId = 3 [default = 0];
  optional int32 replicaIndex = 4;
}

Please add the replication index to org.apache.hadoop.hdds.client.BlockID and org.apache.hadoop.hdds.client.ContainerBlockID
This will be needed when we create container from client, we anyway pass blockID to DN. If index available there, then DN know which index.

org.apache.hadoop.hdds.client.ContainerBlockID doesn't need any modification as it contains the generic id (which points to the replication set) and pipelines (where each element can have replicaIndex information)

Same is true for BlockID some cases we need this information in some cases we don't I propose to add the information only when it's required. We will see it in the next patches with adding the client side / datanode side changes, and we can extend BlockID and create a BlockInstanceId when required...

[elek]

Thanks the review @umamaheswararao, I updated the improved version, PTAL...

[umamaheswararao]

@elek thanks for the update. I will take a look at the latest patch.

pipelines (where each element can have replicaIndex information)

this works as well for me.

[umamaheswararao]

Can we avoid unrelated import organization changes if possible? Otherwise I am +1 on the changes.

[elek]

Sorry, I just realized that I didn't push my changes which restored the import orders. Should be there now...

[umamaheswararao]

The latest changes LGTM

+1

Pending CI run

[elek]

Merging it as CI is green. Thanks the review @umamaheswararao