Skip to content

POC to avoid usage of ReplicationResult - #7528

Closed
ankitsol wants to merge 25 commits into
apache:HBASE-28957from
ankitsol:as.POC
Closed

POC to avoid usage of ReplicationResult#7528
ankitsol wants to merge 25 commits into
apache:HBASE-28957from
ankitsol:as.POC

Conversation

@ankitsol

Copy link
Copy Markdown
Contributor

No description provided.

vinayakphegdeand others added 25 commits September 11, 2025 09:25
…p to External Storage (apache#6633)
* HBASE-28996: Implement Custom ReplicationEndpoint to Enable WAL Backup to External Storage
* fix spotless error
…ckup (apache#6710)
* HBASE-29025: Enhance the full backup command to support continuous backup
* add new check for full backup command regards to continuous backup flag
* minor fixes
…pache#6848)
Signed-off-by: Andor Molnár <andor@apache.org>
Signed-off-by: Wellington Chevreuil <wchevreuil@apache.org>
…ritical backups and propose correct approach (apache#6922)
* improve the logic of backup deletion validation of PITR-critical backups
* add new tests
…he#6717)
Signed-off-by: Andor Molnar <andor@apache.org>
Signed-off-by: Tak Lon (Stephen) Wu <taklwu@apache.org>
…nd (apache#7007)
* Store bulkload files in daywise bucket as well
* Integrate backup WAL cleanup logic with the delete command
* address the review comments
* address the review comments
* address the review comments
* add more unit tests to cover all cases
* address the review comments
)
Signed-off-by: Tak Lon (Stephen) Wu <taklwu@apache.org>
Signed-off-by: Andor Molnár andor@apache.org
Reviewed by: Kota-SH <shanmukhaharipriya@gmail.com>
Reviewed by: Vinayak Hegde <vinayakph123@gmail.com> Reviewed by: Kevin Geiszler <kevin.j.geiszler@gmail.com>
…up is Force Deleted (apache#7090)
Signed-off-by: Tak Lon (Stephen) Wu <taklwu@apache.org> Reviewed by: Kevin Geiszler <kevin.j.geiszler@gmail.com>
apache#7106)
Signed-off-by: Tak Lon (Stephen) Wu <taklwu@apache.org>
Reviewed by: Kota-SH <shanmukhaharipriya@gmail.com>
Reviewed by: Kevin Geiszler <kevin.j.geiszler@gmail.com>
…inuous Backup (apache#7119)
Signed-off-by: Tak Lon (Stephen) Wu <taklwu@apache.org>
Reviewed by: Kevin Geiszler <kevin.j.geiszler@gmail.com>
…che#7045)
Signed-off-by: Tak Lon (Stephen) Wu <taklwu@apache.org>
Reviewed by: Kevin Geiszler <kevin.j.geiszler@gmail.com>
…che#7153)
Signed-off-by: Tak Lon (Stephen) Wu <taklwu@apache.org>
…tries handling to ReplicationEndpoint (apache#7145)
Signed-off-by: Tak Lon (Stephen) Wu <taklwu@apache.org>
…g Incremental Backup (apache#7166)
Signed-off-by: Tak Lon (Stephen) Wu <taklwu@apache.org>
Reviewed by: Kevin Geiszler <kevin.j.geiszler@gmail.com>
…7150)
Signed-off-by: Tak Lon (Stephen) Wu <taklwu@apache.org>
Reviewed by: Kevin Geiszler <kevin.j.geiszler@gmail.com>
…larity (apache#7171)
Signed-off-by: Tak Lon (Stephen) Wu <taklwu@apache.org>
Reviewed by: Kevin Geiszler <kevin.j.geiszler@gmail.com>
Signed-off-by: Tak Lon (Stephen) Wu <taklwu@apache.org>
Signed-off-by: Andor Molnár <andor@apache.org>
…pache#7239)
Signed-off-by: Tak Lon (Stephen) Wu <taklwu@apache.org>
Reviewed by: Kota-SH <shanmukhaharipriya@gmail.com>
apache#7246)
Signed-off-by: Tak Lon (Stephen) Wu <taklwu@apache.org>
…e#7300)
Signed-off-by: Tak Lon (Stephen) Wu <taklwu@apache.org>
Signed-off-by: Andor Molnár andor@apache.org
Reviewed by: Kevin Geiszler <kevin.j.geiszler@gmail.com>
Reviewed by: Kota-SH <shanmukhaharipriya@gmail.com>
… backup (apache#7400)
* Scan WALs to identify bulkload operations for incremental backup
* Update unit test
* Info log
* Minor test fix
* Address review comments
* Spotless apply
* Addressed review comment
* spotless
* Remove log
* Retrigger CI
---------
Co-authored-by: Ankit Solomon <asolomon@cloudera.com>
@Apache-HBase

Copy link
Copy Markdown

🎊 +1 overall

VoteSubsystemRuntimeLogfileComment
+0 🆗reexec0m 35sDocker mode activated.
_ Prechecks _
+1 💚dupname0m 0sNo case conflicting files found.
+0 🆗codespell0m 0scodespell was not available.
+0 🆗detsecrets0m 0sdetect-secrets was not available.
+1 💚@author0m 0sThe patch does not contain any @author tags.
+1 💚hbaseanti0m 0sPatch does not have any anti-patterns.
_ HBASE-28957 Compile Tests _
+0 🆗mvndep0m 15sMaven dependency ordering for branch
+1 💚mvninstall3m 30sHBASE-28957 passed
+1 💚compile4m 3sHBASE-28957 passed
-0 ⚠️checkstyle0m 11s/buildtool-branch-checkstyle-hbase-backup.txtThe patch fails to run checkstyle in hbase-backup
+1 💚spotbugs2m 10sHBASE-28957 passed
+1 💚spotless0m 51sbranch has no errors when running spotless:check.
_ Patch Compile Tests _
+0 🆗mvndep0m 11sMaven dependency ordering for patch
+1 💚mvninstall3m 10sthe patch passed
+1 💚compile4m 2sthe patch passed
+1 💚javac4m 2sthe patch passed
+1 💚blanks0m 0sThe patch has no blanks issues.
-0 ⚠️checkstyle0m 9s/buildtool-patch-checkstyle-hbase-backup.txtThe patch fails to run checkstyle in hbase-backup
+1 💚spotbugs2m 19sthe patch passed
+1 💚hadoopcheck12m 30sPatch does not cause any errors with Hadoop 3.3.6 3.4.0.
+1 💚spotless0m 45spatch has no errors when running spotless:check.
_ Other Tests _
+1 💚asflicense0m 18sThe patch does not generate ASF License warnings.
44m 13s
SubsystemReport/Notes
DockerClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7528/1/artifact/yetus-general-check/output/Dockerfile
GITHUB PR#7528
Optional Testsdupname asflicense javac spotbugs checkstyle codespell detsecrets compile hadoopcheck hbaseanti spotless
unameLinux 27679a0a2f2e 5.4.0-1103-aws #111~18.04.1-Ubuntu SMP Tue May 23 20:04:10 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux
Build toolmaven
Personalitydev-support/hbase-personality.sh
git revisionHBASE-28957 / d2e1a17
Default JavaEclipse Adoptium-17.0.11+9
Max. process+thread count85 (vs. ulimit of 30000)
modulesC: hbase-server hbase-backup U: .
Console outputhttps://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7528/1/console
versionsgit=2.34.1 maven=3.9.8 spotbugs=4.7.3
Powered byApache Yetus 0.15.0 https://yetus.apache.org

This message was automatically generated.

@Apache-HBase

Copy link
Copy Markdown

💔 -1 overall

VoteSubsystemRuntimeLogfileComment
+0 🆗reexec0m 34sDocker mode activated.
-0 ⚠️yetus0m 3sUnprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --author-ignore-list --blanks-eol-ignore-file --blanks-tabs-ignore-file --quick-hadoopcheck
_ Prechecks _
_ HBASE-28957 Compile Tests _
+0 🆗mvndep0m 15sMaven dependency ordering for branch
+1 💚mvninstall3m 32sHBASE-28957 passed
+1 💚compile1m 20sHBASE-28957 passed
+1 💚javadoc0m 44sHBASE-28957 passed
+1 💚shadedjars6m 19sbranch has no errors when building our shaded downstream artifacts.
_ Patch Compile Tests _
+0 🆗mvndep0m 13sMaven dependency ordering for patch
+1 💚mvninstall3m 7sthe patch passed
+1 💚compile1m 19sthe patch passed
+1 💚javac1m 19sthe patch passed
+1 💚javadoc0m 41sthe patch passed
+1 💚shadedjars6m 11spatch has no errors when building our shaded downstream artifacts.
_ Other Tests _
-1 ❌unit233m 6s/patch-unit-hbase-server.txthbase-server in the patch failed.
+1 💚unit18m 51shbase-backup in the patch passed.
281m 31s
SubsystemReport/Notes
DockerClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7528/1/artifact/yetus-jdk17-hadoop3-check/output/Dockerfile
GITHUB PR#7528
Optional Testsjavac javadoc unit compile shadedjars
unameLinux ad59f18e01da 5.4.0-1103-aws #111~18.04.1-Ubuntu SMP Tue May 23 20:04:10 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux
Build toolmaven
Personalitydev-support/hbase-personality.sh
git revisionHBASE-28957 / d2e1a17
Default JavaEclipse Adoptium-17.0.11+9
Test Resultshttps://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7528/1/testReport/
Max. process+thread count4281 (vs. ulimit of 30000)
modulesC: hbase-server hbase-backup U: .
Console outputhttps://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7528/1/console
versionsgit=2.34.1 maven=3.9.8
Powered byApache Yetus 0.15.0 https://yetus.apache.org

This message was automatically generated.

@anmolnaranmolnar left a comment

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.

@ankitsol You might want to raise PR against the HBASE-28957_rebased branch, but otherwise it looks good to me.

replicateContext.entries.stream().map(WAL.Entry::getEdit).flatMap(e -> e.getCells().stream())
.forEach(this::checkCell);
return ReplicationResult.COMMITTED;
getReplicationSource().cleanupHFileRefsAndPersistOffsets(replicateContext.getEntries());

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.

Why we need to call this here?

// replicate the batches to sink side.
parallelReplicate(replicateContext, batches);
return ReplicationResult.COMMITTED;
getReplicationSource().cleanupHFileRefsAndPersistOffsets(replicateContext.getEntries());

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.

OK I think I get why you call this method here, since here we can make sure that the wal entries have been persistent, it is OK for us to persist the offset. But for me, I prefer we follow the old way where call this in ReplicationSourceShipper.

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.

But for me, I prefer we follow the old way where call this in ReplicationSourceShipper.

I think it's doable. @ankitsol ?

/**
* Replicate the given set of entries (in the context) to the other cluster. Can block until all
* the given entries are replicated. Upon this method is returned, all entries that were passed in
* the context are assumed to be persisted in the target cluster.

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.

What I mean is that, we should add a method may be called beforePersistingReplicationOffset, and call it before we call updateLogPosition in ReplicationSourceShipper method. For old implementation, we just do nothing as we can make sure that every thing is persistent, and for S3 based endpoint, we close the writer to persist the data on S3.

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.

You cannot close the writer every time when something was shipped, because closing and re-opening the same stream is a costly operation if even supported. We have to wait for enough data to be shipped (file size limit) or the configured time spent (time limit) before closing the current stream and opening a new one. This is controlled by the replication endpoint itself.

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.

So you also need to change the logic in ReplicationSourceShipper, to not always record the offset after shipping. And I do not think this can 'ONLY' be controlled by replication endpoint, in ReplicationSourceShipper you know the size of the WALEntries, and you also know how much time has elapsed after the last recording, so it is easy to implement the logic?

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's an interesting idea. @vinayakphegde@ankitsol wdyt?

@Apache9 Let's say the ReplicationSourceShipper controls when to record the offset. How would it know which kind of replication endpoint is it working with? Need to record the offset after every shipment or use time/size limit? Shall we make it a new attribute of the endpoints?

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.

I prefer we control it by time/size limit.

Even if the endpoint can persist the data after every shipment, we do not need to record the offset every time right? We just need to make sure that once the ReplicationSourceShipper want to record the offset, all the data before this offset has been persistent. So we can introduce a beforePersistingReplicationOffset method for replication endpoint, if you persist the data after every shipment, you just need to do nothing. If it is S3 based endpoint, we close the output file to persist the data.

In this way, the ReplicationSourceShipper does not need to know whether the endpoint can persist the data or not after every shipment. And in the future, for HBaseInterClusterReplicationEndpoint, we could also introduce some asynchronous mechanism to increase performance.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@Apache9 This seems like a good approach. We also want it to be both time and size based.

I have two questions regarding time based approach

  1. Should this time based count run on a separate thread. Currently in ContinuousBackupReplicationEndpoint we implemented it as a separate thread
  2. Where should be save time/size based context? ReplicationSourceShipper or ReplicationSource, considering 'ReplicationSourceShipper' is itself a thread

CC @anmolnar@vinayakphegde

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.

I think change the default behavior to be size/time based is OK, and we can make size = 0 as no size limit, size = -1 means persisting after every flush, in this way we can make the default size limit as -1 to keep the old behavior.

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.

Sounds good to me.

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.

@Apache9 This seems like a good approach. We also want it to be both time and size based.

I have two questions regarding time based approach

  1. Should this time based count run on a separate thread. Currently in ContinuousBackupReplicationEndpoint we implemented it as a separate thread
  2. Where should be save time/size based context? ReplicationSourceShipper or ReplicationSource, considering 'ReplicationSourceShipper' is itself a thread

CC @anmolnar@vinayakphegde

I think we can put the logic in ReplicationSourceShipper? There is a while loop in the thread, after every shipment, we calculate the size and time, and determine whether we should persist the offset.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

As per our discussion, I have raised a new POC patch #7591

@Apache9 Please take a look at it and share your thoughts

}
}

private ReplicationResult getReplicationResult() {

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.

As said above, we should keep these methods here, and before calling these methods, we call the method in ReplicationEndpoint out to flush data out.

Sign up for freeto 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.

6 participants

@ankitsol@Apache-HBase@Apache9@anmolnar@vinayakphegde@asolomonC