HDDS-11251. Deprecate definitions and remove listTrash and recoverTrash APIs

[devabhishekpal]

What changes were proposed in this pull request?

HDDS-11251. Deprecate definitions and remove listTrash and recoverTrash APIs

Please describe your PR in detail:

• HDDS-2416 was introduced as an OM Trash feature
• Later HDDS-3367 improves upon the above and implements a trash similar to HDFS.
• Since we are no longer using the Trash APIs we can remove them and mark the definitions as deprecated

What is the link to the Apache JIRA

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

How was this patch tested?

Patch was tested via unit tests

[tanvipenumudy]

Thank you @devabhishekpal for working on the patch!

I had a few comments around marking the remaining method implementations with a @Deprecated annotation over removing their implementation/throwing an UnsupportedOperationException, could you please take a look?

[devabhishekpal]

Hello @tanvipenumudy,
Thanks a lot for taking a look at this PR.
I restored the functionality for the methods and marked them as @Deprecated.
Could please take another look?

[ivandika3]

@devabhishekpal Thanks for taking this up.

@tanvipenumudy in my opinion, I would prefer to remove the codes entirely instead of adding @deprecated annotation since these APIs should never be used at all because the feature has not been completed yet. For the protobuf, not sure whether removing the APIs will cause proto.lock compatibility issue. What do you think?

[devabhishekpal]

Hi @ivandika3 , thanks for your inputs.
So previously I was marking the methods as deprecated and in the implementations I was throwing UnsupportedOperationException so that we can know if something is breaking due to the change without affecting anything major.

Would you suggest to completely remove the code?
For the protofiles I am not sure how the lock-files are generated.
I believe as long as we are not affecting existing numbering for other fields we should be good with removing the listTrash and recoverTrash declarations.
@tanvipenumudy what do you think?

[ivandika3]

Would you suggest to completely remove the code?

@devabhishekpal Yes, since these APIs are never implemented and therefore should never be used.

@adoroszlai Kindly need your input.

[adoroszlai]

#6725 removed another "not completely implemented" feature without deprecation. I guess we can remove this one, too.

[ivandika3]

@devabhishekpal Thanks for the update. The current changes LGTM.

Could you help fix the failed tests?

[devabhishekpal]

Hi @ivandika3 @adoroszlai ,
It seems the failure in the build-branch / CI / integration (om) is because the generated proto file has the enum for ListTrash and RecoverTrash present.

But in the code for the test we are missing this from the OmUtils switch-case.

So I tried out with marking the enum numbers as reserved, also tried marking as deprecated.
But it seems in current protoc version this is not supported. So there are the following ways:

• Keep the cases for listTrash and recoverTrash in switch() in OmUtils, but all other references are removed, so unless there is any way in which this switch case is being hit apart from via the test it won't break anything
• Restore the method definitions, throw an UnsupportedOperationException() on the implementations and marking both interface and impl as deprecated. When we are upgraded to protoc3 I believe we can safely remove these defs.

Wanted inputs on this and if there is any other way.
Thanks

[ivandika3]

@devabhishekpal I think we can keep the cases for listTrash and recoverTrash in OmUtils#isReadOnly similar to PurgePaths in #3480

Edit: Can add a // deprecated comment beside the switch case.

Edit: Another way is to update testIsReadOnlyCapturesAllCmdTypeEnums to programatically ignore the deprecated field, from EnumOptions (cmdtype.getDescriptorForType().getOptions()), but I can't seem to get the deprecated field.

[tanvipenumudy]

Thank you @ivandika3 and @adoroszlai for your opinions as well.

I believe it's safe to remove the method definitions since the feature isn't completed yet. However, if these methods were previously in use, we need to ensure compatibility and thoroughly test the same.

[ivandika3]

@devabhishekpal There is one test failure regarding TestOmMetrics, could you help address it? Need to decrement the keyOps assertion due to incNumTrashKeyLists.

https://github.com/devabhishekpal/ozone/actions/runs/10449138799/job/28947749624

[fapifta]

@devabhishekpal Thank you for working on this, I see there is still a test error that seem to be relevant, the om metrics related failure in your fork's CI run.
Can you please take a look at that, and one inline comment from me?

Other than that I think the PR is good to go, unfortunately until we have Hadoop 2.x support, we need to support the proto2 runtime, hence we can not use the reserved keyword to eliminate fileds, and enum values...

[ivandika3]

@devabhishekpal Thanks for the update. LGTM +1.

Let's wait for a green CI.

[ivandika3]

Thanks @devabhishekpal for the patch, and @adoroszlai , @tanvipenumudy , and @fapifta for the reviews.