[Improvement] In Distribution.java correct equals method

[justinmclean]

What would you like to be improved?

The Object class contains an equals method, a method called equals should in general override Object.equals(Object).

How should we improve?

A method called equals should override Object.equals(Object) or that is not its intent it should be renamed.

[simarjeetss]

Hello @justinmclean, can you please provide more details about this issue?

[justinmclean]

Sure, while the code works, it could be improved by implementing equals correctly. See, for example, https://devapo.io/blog/technology/java-hashcode-and-equals-contract on how to do this. Having some unit tests would be a good idea also.

[ajayl83]

@justinmclean i have made improvement to Distribution.java which now overrides equals method from Object with correct impl for Objects.equals! i do have question about Arrays.equals for comparing args... comment

[yuqi1129]

This equal method is specifically designed to compare different instances that implement Distribution interface, so I don't think the changes in https://github.com/datastrato/gravitino/pull/2295 satisfy our needs.

@diqiu50 , please also take a look.

[ajayl83]

@yuqi1129 if i understand correctly Distribution is implemented by DistributionDTO and DistributionImpl , DistributionImpl dont overrride equals method at all , so i am not sure what is the actual requirement here?

[ajayl83]

@yuqi1129 even equals was default method in an interface , so not sure what is the reason to have as default method ! best way is to just have equals implementation in classes that implements Distribution interface.

Classes with implementations always win over default methods, so having a default hashCode or equals can never be invoked and therefore is forbidden

[justinmclean]

Perhaps equals is not the best name for this method, even though it does something similar to the standard equals method?

[yuqi1129]

Perhaps equals is not the best name for this method, even though it does something similar to the standard equals method?

Yes, we can change the name to something else.

[ajayl83]

@justinmclean indeed , so as it was default can you look at the PR as it does not make sense to have method in interface which does same as equals or rename it to something diffrent and have thier own impl do the equals override !

[diqiu50]

This equals method is primarily used to solve compare different instances of DistributionDTO and DistributionImpl. The equals are the best name. If everyone feels that this name is ambiguous, we can also change it.

[justinmclean]

Given equals has a special meaning in Java I would suggest renaming it something like "sameAs" or "equivalent"

[lokendra005]

Kindly review my PR for this issue and let me know if any changes are needed!! @justinmclean

[justinmclean]

There's a compilation issue that needs to be fixed.

[lokendra005]

@justinmclean I have closed the previous pull request because of some build error and raised another pull request. Hope this solves the issue.