Skip to content

Remove warnings from unused code#6354

Merged
ctubbsii merged 1 commit intoapache:mainfrom
ctubbsii:fix-warnings-main
May 1, 2026
Merged

Remove warnings from unused code#6354
ctubbsii merged 1 commit intoapache:mainfrom
ctubbsii:fix-warnings-main

Conversation

@ctubbsii
Copy link
Copy Markdown
Member

  • Remove unused variables, methods, and parameters, causing IDE compiler warnings
  • Also remove warning from unclosed FateStore by placing the new store in a try-with-resources block
  • Also removed one false positive potentially unclosed resource warning in LocatorIT by using the ServerContext for altering the system state for the test instead of using the AccumuloClient as a ClientContext

* Remove unused variables, methods, and parameters, causing IDE compiler
  warnings
* Also remove warning from unclosed FateStore by placing the new store
  in a try-with-resources block
* Also removed one false positive potentially unclosed resource warning
  in LocatorIT by using the ServerContext for altering the system state
  for the test instead of using the AccumuloClient as a ClientContext
@ctubbsii ctubbsii added this to the 4.0.0 milestone Apr 30, 2026
@ctubbsii ctubbsii requested a review from keith-turner April 30, 2026 22:54
@ctubbsii ctubbsii self-assigned this Apr 30, 2026
Comment on lines -203 to -210
private static Optional<TServerInstance> deserializeTserverId(FateKeyType type,
DataInputBuffer buffer) throws IOException {
return switch (type) {
case SPLIT, MERGE, COMPACTION_COMMIT -> Optional.empty();
case TSERVER_SHUTDOWN -> Optional.of(new TServerInstance(buffer.readUTF()));
};
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@keith-turner I think the tServerInstance field became unused after recent distributed Fate changes, but there remains a FateKey constructor that accepts a TServerInstance that is serialized, but then it is never deserialized. I think there's probably some unnecessary stuff going on here, even after my changes, because it doesn't seem useful to have a code path that serializes a TServerInstance when it will never be used.

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.

That code was added in #6224. That code is used to ensure there is only a single fate operations to shutdown a specific tserver running at time. I was just following the pattern of the other code when adding that new fate key type. However it usage differs, we never need to read it. For compaction fate keys those need to be read for dead compaction detection. So since we never read it, it did not need to follow the pattern of the other types. Seems ok to remove it and the parts left behind are still in use.

This code uses the fate key to ensure only one operation is initiated per a tserver. The serialized data is used for the FateKey hashcode and equals functions, so that seems like it will still work for comparing them to dedupe.

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.

In persistent storage the serialized from of the key is used to dedupe by putting it in the Accumulo key for the fate table.

@ctubbsii ctubbsii merged commit 1773204 into apache:main May 1, 2026
9 checks passed
@ctubbsii ctubbsii deleted the fix-warnings-main branch May 1, 2026 00:10
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.

2 participants