[Improvement] LockManager.java comparison using reference equality instead of value equality

[justinmclean]

What would you like to be improved?

In LockManager.java:230: warning: [ReferenceEquality] Comparison using reference equality instead of value equality
if (identifier == ROOT) {

See https://errorprone.info/bugpattern/ReferenceEquality

How should we improve?

Does it mean 'if (Objects.equals(identifier, ROOT)) {' or 'if (identifier.equals(ROOT)) {'?

[YxAc]

In most cases, it is better to use the Objects.equals() method. Because the Objects.equals() method is safer when dealing with null values, it can correctly handle cases where both parameters are null without throwing a NullPointerException.

In the LockManager class, logically speaking, identifier should not be null, so I think identifier.equals(ROOT) is sufficient.

What do you think? Can I fix it with this idea? @justinmclean

[justinmclean]

Either is an improvement over the current code; I trust your judgment as to which is better.

[yuqi1129]

@justinmclean @YxAc
I'm afraid we need to think twice about it, if we just use equals and the users create a name identifier that likes '/', the code may get an unexpected result.

[YxAc]

@justinmclean @YxAc I'm afraid we need to think twice about it, if we just use equals and the users create a name identifier that likes '/', the code may get an unexpected result.

@yuqi1129 , I didn't get what you mean. Can you offer more details? thx

[yuqi1129]

Can you offer more details? thx

What if a user creates a metalake with the name "/", obviously, it equals to the ROOT, but it's not the ROOT.

[YxAc]

Can you offer more details? thx

What if a user creates a metalake with the name "/", obviously, it equals to the ROOT, but it's not the ROOT.

if a user creates a metalake with the name "/", we expect return the root TreeLock? isn't it?
if using equals, return the root TreeLock exactly, isn't it our expected result? @yuqi1129
i think we expected there is only one root treeLock in most cases. Maybe i was wrong.

[yuqi1129]

we expect return the root TreeLock?

No, it's not the ROOT node. ROOT stands for the real root of the tree. Currently, we haven't banned users from naming metalake as '/'.

[YxAc]

we expect return the root TreeLock?

No, it's not the ROOT node. ROOT stands for the real root of the tree. Currently, we haven't banned users from naming metalake as '/'.

if we use ==, it indeed not return the ROOT node, but i thought we should return the ROOT node, otherwise it's confused here.

if ==(reference equality) is deliberately used here to distinguish whether it is the real ROOT node, so this problem can be turned off? @yuqi1129

[yuqi1129]

if ==(reference equality) is deliberately used here to distinguish whether it is the real ROOT node, so this problem can be turned off?

Yes, I use == (reference equality) deliberately, but any improvement and optimization is welcome, maybe we can come up with a better solution here.

[justinmclean]

If there was a good reason for using reference equality and it was deliberately done, then a comment should be added stating why this was done.