Skip to content

TODO Cleanup #6509

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 8 commits into
base: main
Choose a base branch
from
Open

TODO Cleanup #6509

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
773f26c
Obscure keyExtents in outstanding migrations output to remove TODO
Amemeda Aug 20, 2026
6a3cfe4
Combine streams into the logger
Amemeda Aug 21, 2026
4d7afaa
Fix stream to collect into 1 set
Amemeda Aug 21, 2026
6428ce4
Fix return obscured keyExtent
Amemeda Aug 21, 2026
8af6a3f
Remove potentially irrelevant TODO over inconsistent exception
Amemeda Aug 24, 2026
164cf0f
Remove resolved TODO from Value.java
Amemeda Aug 27, 2026
cab5e17
Update core/src/main/java/org/apache/accumulo/core/spi/balancer/util/…
Amemeda Sep 2, 2026
1dc6e3d
Formatting
Amemeda Sep 3, 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 core/src/main/java/org/apache/accumulo/core/clientImpl/ClientContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,11 +334,6 @@ public ClientContext(ClientInfo info, AccumuloConfiguration serverConf,
});

zkLockChecker = memoize(() -> {
// make this use its own ZooSession and ZooCache, because this is used by the
// tablet location cache, which is a static singleton reused by multiple clients
// so, it can't rely on being able to continue to use the same client's ZooCache,
// because that client could be closed, and its ZooSession also closed
// this needs to be fixed; TODO https://github.com/apache/accumulo/issues/2301
var zk = info.getZooKeeperSupplier(ZookeeperLockChecker.class.getSimpleName(),
ZooUtil.getRoot(getInstanceID())).get();
return new ZookeeperLockChecker(new ZooCache(zk, Set.of(Constants.ZTSERVERS)));
Comment on lines 336 to 339

@Amemeda Amemeda Aug 25, 2026 •
edited
Loading

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.

Attaching comment from issue discussing the relevance of this TODO. It looks like it is no longer relevant but could use @ctubbsii opinion.
#2699 (comment)

@ctubbsii ctubbsii Sep 3, 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 haven't looked over the whole PR, but I think it's correct that this comment is no longer applicable on the main branch, as it is currently written, since the location cache is not shared across clients anymore after #5282.

However, the code is still creating its own ZooCache for some reason, instead of using the context-specific one. So either:

  1. that is no longer necessary and it can use the one already in the ClientContext, and the comment can be removed after that refactor, or
  2. it is still necessary because the lock checker is leaking into threads that can survive after closing the ClientContext, in which case the comment needs to be updated to reflect the current justification.

I don't know which scenario is the case.

Expand Down
15 changes: 10 additions & 5 deletions ...ain/java/org/apache/accumulo/core/spi/balancer/util/ThrottledBalancerProblemReporter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import java.util.stream.Collectors;

import org.apache.accumulo.core.data.TabletId;
import org.apache.accumulo.core.dataImpl.KeyExtent;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand Down Expand Up @@ -77,13 +78,17 @@ public void setMigrations(Set<TabletId> migrations) {
@Override
public void report() {
log.warn("Not balancing due to {} outstanding migrations.", migrations.size());
/*
* TODO ACCUMULO-2938 redact key extents in this output to avoid leaking protected
* information.
*/

// convert each tabletId in migrations to keyExtent
Set<KeyExtent> keyExtents = migrations.stream().map(tabletId -> {
KeyExtent extent = KeyExtent.fromTabletId(tabletId);
extent.obscured();
return extent;
}).collect(Collectors.toSet());

if (log.isDebugEnabled()) {
log.debug("Sample up to 10 outstanding migrations: {}",
migrations.stream().limit(10).map(String::valueOf).collect(Collectors.joining(", ")));
keyExtents.stream().limit(10).map(String::valueOf).collect(Collectors.joining(", ")));
Comment thread
DomGarguilo marked this conversation as resolved.
Outdated
Comment thread
Amemeda marked this conversation as resolved.
Outdated
}
// Now that we've reported, clear out the migrations list so we don't hold it in memory.
migrations = Collections.emptySet();
Expand Down