Skip to content

Optimize ZooKeeper read requests and remove redundant checks#8714

Merged
reiabreu merged 2 commits into
apache:masterfrom
GGraziadei:8712-remove-redundant-zk-exists-check
May 31, 2026
Merged

Optimize ZooKeeper read requests and remove redundant checks#8714
reiabreu merged 2 commits into
apache:masterfrom
GGraziadei:8712-remove-redundant-zk-exists-check

Conversation

@GGraziadei
Copy link
Copy Markdown
Contributor

What is the purpose of the change

Refactoring ZooKeeper reads to optimize request volume. Removed unnecessary .exists() calls by handling missing nodes reactively. More details in the linked issue.

Proposed reading pattern

GIVEN path, wantWatch:
      result <- READ path (requesting a watch if wantWatch)  // request #1

      if result is present:
          return result.value   // watch already set if requested
      else:  // node is absent (e.g. `NoNodeException`)
          if wantWatch:
              WATCH path  // request #2, only here
          return EMPTY
      on any other error:
          FAIL 

How was the change tested

  • Unit tests
  • Added new test cases for node delete-delete conflicts and gestVersion

In the context of #8712


state.close();
}
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There is no test case that does: getData(path, watch=true) on a missing node, then creates the node, and verifies the callback was triggered,
What do you thinking about adding it?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thank you! I've committed the missing test case.
This test is crucial for preventing regressions. It verifies that if a getData call to zk fails with watched clause enabled, the fallback mechanism successfully registers the watcher manually (calling the method existsNode).

@rzo1 rzo1 added this to the 3.0.0 milestone May 31, 2026
@reiabreu reiabreu merged commit ad5789a into apache:master May 31, 2026
11 checks passed
@reiabreu
Copy link
Copy Markdown
Contributor

Thanks for this contribution @GGraziadei

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.

3 participants