Re: [PR] [fix][meta] Fix underreplication locks left behind when closing by handling each deletion independently [pulsar]

["Denovo1998 (via GitHub)" <gi...@apache.org>]

Denovo1998 commented on code in PR #26674:
URL: https://github.com/apache/pulsar/pull/26674#discussion_r4123223767


##########
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/bookkeeper/PulsarLedgerUnderreplicationManager.java:
##########
@@ -690,23 +690,35 @@ public void releaseUnderreplicatedLedger(long ledgerId) throws ReplicationExcept
     public void close() throws ReplicationException.UnavailableException {
         log.debug("close()");
         notificationCallbackExecutor.shutdownNow();
-        try {
-            for (Map.Entry<Long, Lock> e : heldLocks.entrySet()) {
+        ReplicationException.UnavailableException failure = null;
+        for (Map.Entry<Long, Lock> e : heldLocks.entrySet()) {
+            try {
                 store.delete(e.getValue().getLockPath(), Optional.empty())
                         .get(BLOCKING_CALL_TIMEOUT, MILLISECONDS);

Review Comment:
   The ownership-fencing limitation looks reasonable to keep out of this PR, but I think the interrupted path has the same ambiguity as the timeout path.
   
   Once `store.delete()` has been submitted, `get()` timing out or being interrupted doesn't mean the delete didn't complete. If the lock is recreated by another owner before a later `close()`, the retained stale entry can still delete that new lock because the retry uses `Optional.empty()`.
   
   This also applies to `releaseUnderreplicatedLedger()`. Could we track this as a follow-up for both paths? I don't think it needs to block the cleanup fix here, but it would be good not to treat interruption as an unambiguous failed delete.



##########
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/bookkeeper/PulsarLedgerUnderreplicationManager.java:
##########
@@ -690,23 +690,35 @@ public void releaseUnderreplicatedLedger(long ledgerId) throws ReplicationExcept
     public void close() throws ReplicationException.UnavailableException {
         log.debug("close()");
         notificationCallbackExecutor.shutdownNow();
-        try {
-            for (Map.Entry<Long, Lock> e : heldLocks.entrySet()) {
+        ReplicationException.UnavailableException failure = null;
+        for (Map.Entry<Long, Lock> e : heldLocks.entrySet()) {
+            try {
                 store.delete(e.getValue().getLockPath(), Optional.empty())
                         .get(BLOCKING_CALL_TIMEOUT, MILLISECONDS);

Review Comment:
   Could we avoid applying the full blocking timeout to every lock here?
   
   `BLOCKING_CALL_TIMEOUT` is 30 seconds, and `ReplicationWorker` can have multiple locks in `heldLocks` while failed-ledger releases are deferred. During shutdown those timer tasks are cancelled and `underreplicationManager.close()` is used to clean up the remaining locks.
   
   With this loop, if the metadata store stops responding, shutdown can now take `heldLocks.size() * 30s`. The old implementation would stop after the first timeout.
   
   Would it make sense to use one overall deadline for `close()`, or issue the deletes first and wait for them with one bounded timeout? That should still let us attempt cleanup for every lock without making the shutdown time grow linearly with the number of held locks.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org