HBASE-28996: Implement Custom ReplicationEndpoint to Enable WAL Backup to External Storage (Option 3) #6633
+1,303
−60
Conversation
This file contains 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
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
@vinayakphegde Please make this patch ready for review, if you're done with it. |
anmolnar
reviewed
Feb 10, 2025
There was a problem hiding this comment.
Generally looks good to me, though I'm not sure if would split this patch into two by separating the framework related changes.
I would add a few trace level log lines for debugging backup events. Usually I just leave my custom logging in the code that I used for developing the patch. They would be useful for future debugging.
| import org.slf4j.LoggerFactory; | ||
|
|
||
| @InterfaceAudience.Private | ||
| public class BackupFileSystemManager { |
There was a problem hiding this comment.
Please add javadoc comment to this class.
| public class ContinuousBackupReplicationEndpoint extends BaseReplicationEndpoint { | ||
| private static final Logger LOG = | ||
| LoggerFactory.getLogger(ContinuousBackupReplicationEndpoint.class); | ||
| public static final String CONF_PEER_UUID = "hbase.backup.wal.replication.peerUUID"; |
There was a problem hiding this comment.
Do we need this configurable? Shouldn't we just generate randomly?
There was a problem hiding this comment.
No, I think for a particular target, it should be unique and same all the time.
https://github.com/apache/hbase/blob/master/hbase-server/src/main/java/org/apache/hadoop/hbase/replication/ReplicationEndpoint.java#L140-#L146
| if (fsDataOutputstream == null) { | ||
| return; // Presume closed | ||
| } | ||
| fsDataOutputstream.flush(); |
There was a problem hiding this comment.
Does this have any effect on S3?
Don't remove, just asking.
There was a problem hiding this comment.
as far as I understand,
When multipart upload is enabled and the buffer type is set to disk:
- Data is written to the active block using the write() API.
- When flush() is called, it flushes the current block, which in this case writes to the underlying disk.
- When the current block is full during a write() operation, the upload for that block is initiated asynchronously.
- Finally, when close() is called, it uploads the last block, waits for all block uploads to complete, and then finalizes the upload process.
so, flush will persist the data of the current block to the disk.
But it doesn't actually sync the data to s3.
so, I think we don't need that.
also, we don't need to call the sync method as well I guess.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
No description provided.