HDDS-8418. Lease Manager unsafe method fix

[sumitagrawl]

What changes were proposed in this pull request?

registerCallback in Lease is not safe if done after lease expiry in parallel, so this callback will never be registered. Removed this method exposer.

What is the link to the Apache JIRA

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

How was this patch tested?

UT case and IT for checking any impact

[adoroszlai]

@sumitagrawl there are multiple test failures, please check

[adoroszlai]

@sumitagrawl thanks for working on this. It's not clear what the problem was and how the patch is supposed to fix it.

I suspect intermittent failure in TestCloseContainerEventHandler could be the result of the problem being fixed:

[INFO] Running org.apache.hadoop.hdds.scm.container.TestCloseContainerEventHandler
Error:  Tests run: 5, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 6.357 s <<< FAILURE! - in org.apache.hadoop.hdds.scm.container.TestCloseContainerEventHandler
Error:  org.apache.hadoop.hdds.scm.container.TestCloseContainerEventHandler.testCloseContainerWithDelayByLeaseManager  Time elapsed: 4.528 s  <<< FAILURE!
Wanted but not invoked:
eventPublisher.fireEvent(
    TypedEvent{payloadType=CommandForDatanode, name='Datanode_Command'},
    <Capturing argument>
);
-> at org.apache.hadoop.hdds.scm.container.TestCloseContainerEventHandler.testCloseContainerWithDelayByLeaseManager(TestCloseContainerEventHandler.java:151)
Actually, there were zero interactions with this mock.

Synchronization in Lease is very mixed after this patch:

• write to expired is synchronized (invalidate()), but read is not (hasExpired()), except when called from other synchronized method
• callbacks is a synchronizedList, item addition is also synchronized on Lease (registerCallback()), but getCallbacks() returns the list as is, and iteration happens without either lock
• leaseTimeout is atomic, write is also synchronized (renew()), but read is not (getLeaseLifeTime())

Several methods called from LeaseManager are also protected by synchronization in that class.

Instead of adding a few synchronized keywords here and there, it would be great to make it consistent.

[szetszwo]

The bug is the acquire methods returning a Lease and then registerCallBack later. It seems the current change does not prevent that. We should pass the callbacks to acquire at the same time in order to fix the bug.

[sadanand48]

I didn't get this part, what is the impact of changing void-> Void and returning null

[sumitagrawl]

Callback as getting registered has return type is Callback with "Void" Object, so this gives error if return type mismatch,

Syntax for acquire: public synchronized Lease<T> acquire( T resource, Callable<Void> callback)

Below is error if we keep void:
Bad return type in lambda expression: void cannot be converted to Void

[sadanand48]

Got it , looks like autoboxing won't happen for void type. Thanks for explaining

[sadanand48]

LGTM

[sadanand48]

Thanks @sumitagrawl for the fix, @adoroszlai @szetszwo for reviewing.