-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Unload bundle: close topic forcefully and enable bundle on ownership removal failure #164
Conversation
CLA is valid! |
2 similar comments
CLA is valid! |
CLA is valid! |
@merlimat can you also please take a look of this PR. |
@saandrews @merlimat : can you please review this PR. |
👍 |
@@ -101,16 +101,29 @@ public void handleUnloadRequest(PulsarService pulsar) throws Exception { | |||
this.nsLock.writeLock().unlock(); | |||
} | |||
|
|||
int unloadedTopics = 0; | |||
int unloadedTopics = -1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-1 is a bit confusing here
try { | ||
unloadedTopics = pulsar.getBrokerService().unloadServiceUnit(bundle).get(); | ||
} catch (Exception e) { | ||
// ignore topic-close failure to unload bundle |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also make sure the MLs are removed from the ML factory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
while closing the topic we disconnects producers, subscribers and replicator and if it is successful then we remove it from the MLFactory immediately.
Disconnecting prod,sub,repl should be clean but if not for one of the topic then additionally we can schedule a retry only for unloading-bundle : if there was a failure while closing any topic and if we successfully remove the ownership-node later.
any thought?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was saying to verify that ML.close() will remove the ML from the factory even in the presence of errors. Errors could happen when closing the cursor or closing the last data ledger.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, ML.close() removes ML from MLFactory first and then tries to close ledger the cursors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Bumps `log4j2.version` from 2.10.0 to 2.13.3. Updates `log4j-core` from 2.10.0 to 2.13.3 Updates `log4j-slf4j-impl` from 2.10.0 to 2.13.3
Motivation
while unloading namespace bundle if
topic.close()
fails then bundle gets stuck by staying indisable
state. Therefore, unload bundle should close topic forcefully and failure in deletion of bundle-ownership node should revert bundle state toenable
back so, it can serve new topics again.Modifications
Result
bundle will not be in stuck state in case of failure in unloading namespace bundle.