Skip to content

Remove TODOs from RootTabletMutatorImpl.java #6494

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.

Sign up for GitHub

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

Open
Amemeda wants to merge 6 commits into
base: main
Choose a base branch
from
+8 −20
Open

Remove TODOs from RootTabletMutatorImpl.java #6494

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a10563d
Remove TODO's from RootTabletMutatorImpl.java
Amemeda Jul 29, 2026
83ac106
Wait for ZROOT_TABLET to empty before calling clear() again
Amemeda Jul 29, 2026
429af37
Add temporary wait for ZROOT_TABLET to clear
Amemeda Jul 30, 2026
734e724
Add Thread.sleep after 2 zooCache clears to remove 2 TODOs
Amemeda Jul 30, 2026
6b0ae0f
Remove zooCache.clear()s to avoid redundancy
Amemeda Aug 5, 2026
43b853c
Add back 1 zooCache().clear() in 2 files where previously removed
Amemeda Aug 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions server/base/src/main/java/org/apache/accumulo/server/metadata/RootConditionalWriter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,6 @@ public Result write(ConditionalMutation mutation) {

ServerConditionalMutation scm = new ServerConditionalMutation(tcm);

context.getZooCache().clear(RootTable.ZROOT_TABLET);

List<ServerConditionalMutation> okMutations = new ArrayList<>();
List<TCMResult> results = new ArrayList<>();

Expand Down Expand Up @@ -129,10 +127,7 @@ public Result write(ConditionalMutation mutation) {
} catch (Exception e) {
throw new RuntimeException(e);
}

// TODO this is racy...
context.getZooCache().clear(RootTable.ZROOT_TABLET);

@ctubbsii ctubbsii Aug 6, 2026

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.

I think other code may benefit from having this stay here. I don't really know what the comment refers to or whether it matters.

@ctubbsii ctubbsii Aug 6, 2026

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.

@dlmarion has previously worked on the ZooCache stuff and might have more insight into whether these cache evictions are useful.

@dlmarion dlmarion Aug 7, 2026

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.

The code above just mutated the root tablet metadata in ZooKeeper. IIRC, the ZCacheWatcher will fire at some point in the future and receive a NodeDataChanged event, and it will clear the cache using this same type of call. However, until that Watcher fires, ZooCache will contain and return the old stale data.

I think the comment here about this being a race condition was suggesting that the clear may/will happen twice potentially in quick succession. However, I think it's still required here because we don't know when, the Watcher will fire.

I think this clear needs to stay.

@ctubbsii ctubbsii Aug 7, 2026 •
edited
Loading

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.

@dlmarion I'll believe you if you say yes, but I was thinking we probably don't need the clear before the update, since we're not reading from ZooCache to perform the update, and there's no point in clearing it until after we're done making the changes. Are you sure we need the one before hand?

@dlmarion dlmarion Aug 10, 2026

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.

I don't think we need the one before the mutation, but I think we do need the one after.


return getResult(okMutations, results, mutation);
}

Expand Down
23 changes: 8 additions & 15 deletions server/base/src/main/java/org/apache/accumulo/server/metadata/RootTabletMutatorImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,24 +97,17 @@ public void mutate() {
}

try {
context.getZooSession().asReaderWriter().mutateExisting(RootTable.ZROOT_TABLET, currVal -> {
String currJson = new String(currVal, UTF_8);
var rtm = new RootTabletMetadata(currJson);
rtm.update(mutation);
String newJson = rtm.toJson();
log.debug("mutation: from:[{}] to: [{}]", currJson, newJson);
return newJson.getBytes(UTF_8);
});

context.getZooCache().clear(RootTable.ZROOT_TABLET);

// TODO examine implementation of getZooReaderWriter().mutate()
// TODO for efficiency this should maybe call mutateExisting
context.getZooSession().asReaderWriter().mutateOrCreate(RootTable.ZROOT_TABLET, new byte[0],
currVal -> {
String currJson = new String(currVal, UTF_8);
var rtm = new RootTabletMetadata(currJson);
rtm.update(mutation);
String newJson = rtm.toJson();
log.debug("mutation: from:[{}] to: [{}]", currJson, newJson);
return newJson.getBytes(UTF_8);
});

// TODO this is racy...
context.getZooCache().clear(RootTable.ZROOT_TABLET);
Comment thread
Amemeda marked this conversation as resolved.

if (closeAfterMutate != null) {
closeAfterMutate.close();
}
Expand Down