HBASE-29642 Active cluster file is not being updated after promoting … #7437
+136
−0
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
…a new active cluster
This comment has been minimized.
This comment has been minimized.
…a new active cluster
This comment has been minimized.
This comment has been minimized.
|
🎊 +1 overall
This message was automatically generated. |
anmolnar
requested changes
Nov 4, 2025
Contributor
There was a problem hiding this comment.
I don't really understand the reasoning behind all this complicated logic. To my understanding the task is pretty simple:
Monitor the transition of read-only property and do the following:
- if it transitions from R/W -> R/O, delete the file,
- if R/O -> R/W, create the file.
If the above operation fails, log it and try to notify the user somehow.
That's basically it. No backoff logic, no retry logic is needed for now. User will fix the problem and restarts if needed.
|
|
||
| // Not timed out, log and sleep before retrying | ||
| LOG.warn( | ||
| "Failed to recreate active cluster ID file: {}. Retrying in {}ms... " |
Contributor
There was a problem hiding this comment.
I don't think you need to retry to create the file. Try it only once and log error if it failed. There's no such thing that a filesystem is unavailable for some reason, but will become available at some point and we wait for it. It should be available at all times or something is wrong.
| * specified permissions and overwrite flag. | ||
| * @param fs The FileSystem | ||
| * @param path The file path to recreate | ||
| * @param recursive If true, delete recursively (for a directory) |
Contributor
There was a problem hiding this comment.
This is about a single file. 'Recursive' doesn't make sense to me here.
| * @param path The file path to recreate | ||
| * @param recursive If true, delete recursively (for a directory) | ||
| * @param perm The FsPermission to apply to the new file | ||
| * @param overwrite If true, overwrite an existing file (though it should be deleted by this |
Contributor
There was a problem hiding this comment.
You're trying to recreate a file. This should be always true.
| FsPermission perm, boolean overwrite) throws IOException { | ||
| LOG.debug("Attempting to delete path {} (recursive={}) for recreation.", path, recursive); | ||
| try { | ||
| if (!fs.delete(path, recursive) && fs.exists(path)) { |
Contributor
There was a problem hiding this comment.
fs.exists() is redundant. fs.delete() should give your a clear result about whether the delete operation was successful or not.
| throw new IOException("Failed to delete " + path + " for recreation. " | ||
| + "delete() returned false and path still exists."); | ||
| } | ||
| } catch (IOException e) { |
Contributor
There was a problem hiding this comment.
Why do you need this all complicated logic? Delete either failed or successful, right?
| overwrite); | ||
|
|
||
| // Create the new file with the specified permissions | ||
| return create(fs, path, perm, overwrite); |
Contributor
There was a problem hiding this comment.
You should not recreate the file when read-only mode is turned on. In that case you only need to delete the file and let the active cluster a new. How do cover that scenario?
|
💔 -1 overall
This message was automatically generated. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
…a new active cluster
This change