Skip to content

[server] fix zk notification purge race#3752

Open
gyang94 wants to merge 2 commits into
apache:mainfrom
gyang94:fix-zk-notification-purge-race
Open

[server] fix zk notification purge race#3752
gyang94 wants to merge 2 commits into
apache:mainfrom
gyang94:fix-zk-notification-purge-race

Conversation

@gyang94

@gyang94 gyang94 commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Purpose

Linked issue: close #3753

Brief change log

empty check for zk stats

Tests

API and Format

Documentation

@gyang94 gyang94 changed the title fix zk notification purge race [server] fix zk notification purge race Jul 23, 2026

@fresh-borzoni fresh-borzoni left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gyang94 Thank you, fix is sound, some ideas about the test below, PTAL

The test passes even with the fix reverted because our test config has the root logger OFF, so the appender never gets the ERROR and noneMatch on an empty list is always true.
So a couple of suggestions:

  1. mockito isn't really needed: the handler runs before the purge and they share the children list, so a handler that deletes the notification node gives the purge a stale list, the exact race from the issue.
    a sketch:
 TestingNotificationHandler handler =
         new TestingNotificationHandler() {
             @Override
             public void processNotification(byte[] notification) {
                 super.processNotification(notification);
                 deleteAllNotificationNodes(); // purge now sees a stale list
             }
         };
  1. with root OFF the appender only works through its own LoggerConfig, smth like this:
LoggerContext ctx = (LoggerContext) LogManager.getContext(false);
Configuration cfg = ctx.getConfiguration();
LoggerConfig lc = new LoggerConfig(ZkNodeChangeNotificationWatcher.class.getName(), Level.DEBUG, false);
lc.addAppender(appender, Level.DEBUG, null);
cfg.addLogger(lc.getName(), lc);
ctx.updateLoggers();

and assert the debug message is present rather than the error absent, so it can't go vacuous again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Concurrent ZK notification cleanup throws NoSuchElementException

2 participants