Remove obsolete Metadata.FORMAT field and usages #129519
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The only production usage is for cleaning up all global state files. It is replaced by directly calling the relevant method without creating the FORAMT instance. Test only usages are either replaced by equivalent method calls or dropped. Relates: elastic#114698
ywangd
added
>non-issue
:Distributed Coordination/Cluster Coordination
elasticsearchmachine
added
the
Team:Distributed Coordination
|
Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination) |
ywangd
commented
Jun 17, 2025
| * | ||
| * @throws WriteStateException if exception when writing state occurs. See also {@link WriteStateException#isDirty()} | ||
| */ | ||
| public static void writeManifestAndCleanup(NodeEnvironment nodeEnv, String reason, Manifest manifest) throws WriteStateException { |
There was a problem hiding this comment.
This method is not used before this PR. So I simply removing it here.
DaveCTurner
reviewed
Jun 17, 2025
| public void unreferenceAll() throws IOException { | ||
| Manifest.FORMAT.writeAndCleanup(Manifest.empty(), nodeEnv.nodeDataPaths()); // write empty file so that indices become unreferenced | ||
| Metadata.FORMAT.cleanupOldFiles(Long.MAX_VALUE, nodeEnv.nodeDataPaths()); | ||
| MetadataStateFormat.cleanupOldFiles(GLOBAL_STATE_FILE_PREFIX, Long.MAX_VALUE, nodeEnv.nodeDataPaths(), NIOFSDirectory::new); |
There was a problem hiding this comment.
Should we just delete the global-*.st blobs directly? No need to go via NIOFSDirectory or anything.
ywangd
commented
Jun 18, 2025
| logger.trace("cleanupOldFiles: cleaning up {} for global state files", location); | ||
| final Path stateLocation = location.resolve(MetadataStateFormat.STATE_DIR_NAME); | ||
| try (var paths = Files.list(stateLocation)) { | ||
| paths.filter(file -> file.getFileName().toString().startsWith(GLOBAL_STATE_FILE_PREFIX)).forEach(file -> { |
There was a problem hiding this comment.
This no longer filters based on Long.MAX_VALUE anymore. I think in practice, it is the same as deleting all global-* files. But please let me know if you prefer to keep it.
There was a problem hiding this comment.
I prefer this, the Long.MAX_VALUE filter was kind of a hack to make it delete everything.
| public void unreferenceAll() throws IOException { | ||
| Manifest.FORMAT.writeAndCleanup(Manifest.empty(), nodeEnv.nodeDataPaths()); // write empty file so that indices become unreferenced | ||
| Metadata.FORMAT.cleanupOldFiles(Long.MAX_VALUE, nodeEnv.nodeDataPaths()); | ||
| MetadataStateFormat.cleanupOldFiles(GLOBAL_STATE_FILE_PREFIX, Long.MAX_VALUE, nodeEnv.nodeDataPaths(), NIOFSDirectory::new); |
|
@elasticmachine update branch |
DaveCTurner
approved these changes
Jun 18, 2025
| logger.trace("cleanupOldFiles: cleaning up {} for global state files", location); | ||
| final Path stateLocation = location.resolve(MetadataStateFormat.STATE_DIR_NAME); | ||
| try (var paths = Files.list(stateLocation)) { | ||
| paths.filter(file -> file.getFileName().toString().startsWith(GLOBAL_STATE_FILE_PREFIX)).forEach(file -> { |
There was a problem hiding this comment.
I prefer this, the Long.MAX_VALUE filter was kind of a hack to make it delete everything.
|
@elasticmachine update branch |
ywangd
added
the
auto-merge-without-approval
kderusso
pushed a commit
to kderusso/elasticsearch
that referenced
this pull request
Jun 23, 2025
The only production usage is for cleaning up all global state files. It is replaced by directly calling the relevant method without creating the FORAMT instance. Test only usages are either replaced by equivalent method calls or dropped. Relates: elastic#114698
mridula-s109
pushed a commit
to mridula-s109/elasticsearch
that referenced
this pull request
Jun 25, 2025
The only production usage is for cleaning up all global state files. It is replaced by directly calling the relevant method without creating the FORAMT instance. Test only usages are either replaced by equivalent method calls or dropped. Relates: elastic#114698
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The only production usage is for cleaning up all global state files. It is replaced by directly calling the relevant method without creating the FORAMT instance. Test only usages are either replaced by equivalent method calls or dropped.
Relates: #114698