HDDS-7098. Provide a way for admin to identify all unhealthy container replicas

[mladjan-gadzic]

What changes were proposed in this pull request?

Add state field for container replica response for Recon API.

What is the link to the Apache JIRA

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

How was this patch tested?

• manual
• unit

[mladjan-gadzic]

An idea was to pull an enum from https://github.com/mladjan-gadzic/ozone/blob/HDDS-7098/hadoop-hdds/interface-server/src/main/proto/ScmServerDatanodeHeartbeatProtocol.proto#L213 and use it here in order not to maintain same enum in two places but I could not make it. Because of that I decided not to use enum at all and stick with string instead.

[jojochuang]

will it be backward compatible if it's a required field, or is backward compat not a concern here?

[mladjan-gadzic]

I'd say backward compatibility is not a concern here.

[jojochuang]

It would break rolling upgrade, or when recon and SCM are in different versions.

I see that rolling upgrade is not yet a thing, so I am okay with it.

[kerneltime]

There needs to be a powerful reason for making the proto message required. I would change this to optional. If the code depends on it, it should handle it rather than having proto layer throw an exception on parsing.

[kerneltime]

cc @adoroszlai can you please take a look?

[jojochuang]

It would be ideal to reuse the State Enum type.
Would the answer in this stackoverflow article help address the import problem? https://stackoverflow.com/questions/66524793/how-to-use-the-enum-of-one-proto-file-in-another-file

[mladjan-gadzic]

It would be ideal to reuse the State Enum type. Would the answer in this stackoverflow article help address the import problem? https://stackoverflow.com/questions/66524793/how-to-use-the-enum-of-one-proto-file-in-another-file

@jojochuang thanks for the review. I like that idea as well. I've tried it, but there is an issue with backward compatibility: [ERROR] Failed to execute goal com.salesforce.servicelibs:proto-backwards-compatibility:1.0.7:backwards-compatibility-check (default) on project hdds-interface-server: Backwards compatibility check failed! You can override this by specifying allowBreakingChanges=true -> [Help 1]. I'd say we should not break it, especially because I am not aware of the consequences it could have on the system

[adoroszlai]

It would be ideal to reuse the State Enum type. Would the answer in this stackoverflow article help address the import problem? https://stackoverflow.com/questions/66524793/how-to-use-the-enum-of-one-proto-file-in-another-file

I've tried it, but there is an issue with backward compatibility

Does the compatibility check also fail if the field is optional instead of required?

[mladjan-gadzic]

It would be ideal to reuse the State Enum type. Would the answer in this stackoverflow article help address the import problem? https://stackoverflow.com/questions/66524793/how-to-use-the-enum-of-one-proto-file-in-another-file

I've tried it, but there is an issue with backward compatibility

Does the compatibility check also fail if the field is optional instead of required?

Yes, it does. Backward compatibility check fails for hdds-interface-server from which I've tried to pull an enum. Furthermore I tried using optional there, but no avail.

[errose28]

Not being able to use the enum is somewhat unfortunate, but there is a precedent for using the string state in hdds.proto from SCMContainerReplicaProto, which likely had to use a string for the same reason. I have one question inline, but overall LGTM if others are satisfied with the current state of the protos.

[errose28]

Thanks for the update @mladjan-gadzic LGTM. Does anyone involved in the earlier proto discussion have outstanding concerns? @kerneltime @jojochuang @adoroszlai

[mladjan-gadzic]

@errose28 thanks for the review!

[adoroszlai]

Failure of TestOzoneManagerHASnapshot is unrelated and it has been fixed on master since then.

[adoroszlai]

Thanks @mladjan-gadzic for the patch, @devmadhuu, @errose28, @jojochuang, @kerneltime for the review.