HDDS-4769. Simplify insert operation of ContainerAttribute

[symious]

What changes were proposed in this pull request?

• Keep the original value in ContainerAttribute when inserting ContainerId.

What is the link to the Apache JIRA

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

How was this patch tested?

Unit test in TestContainerAttribute.testInsert() validates the correctness.

[symious]

@elek @adoroszlai
Could you have a review? Split the update operation to make it more readable. And if we are going to add more information to ContainerID, it will be easier to modify the update operation.

[adoroszlai]

Thanks @symious for finding room for improvement.

Maybe I'm missing something, but it seems most of the logic in insert (except the preconditions) could be simplified to:

    return attributeMap
        .computeIfAbsent(key, any -> new TreeSet<>())
        .add(value);

[symious]

I think it will return false when the ContainerID already exists in the NavigableSet?

[symious]

I think normally the operation of insert needs to update the old value in the set, but for ContainerID, since it doesn't contain information other than ID, it's kind of a special case here that we don't need to update the old value.

[symious]

But I'm not sure if there will be new fields added in ContainerID, maybe timestamps or versionID, then the special case for ContainerID would not exists. I added the method of update(key, value) so that we can update more easily.

[adoroszlai]

I think it will return false when the ContainerID already exists in the NavigableSet?

You're right. Then:

attributeMap
    .computeIfAbsent(key, any -> new TreeSet<>())
    .add(value);
return true;

Anyway, I don't think we need to prepare now for fields maybe being added to containerID in the future.

[symious]

@adoroszlai The insert operation has been simplified according to your last suggestion, could you have a check?

[adoroszlai]

The insert operation has been simplified according to your last suggestion, could you have a check?

Thanks @symious for the update, it LGTM. I would like someone else to review it, since it's exactly my suggestion and I could be wrong.

[elek]

LGTM.

To be honest I don't understand the motivation behind the original, complicated code.

The new version seems to be the same.

[elek]

Merging it. Thanks the patch @symious and the review @adoroszlai