Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

HBASE-28608 More sensible client meta operation timeout default #6000

Merged
merged 1 commit into from
Nov 21, 2024
Merged

HBASE-28608 More sensible client meta operation timeout default #6000

merged 1 commit into from
Nov 21, 2024

Conversation

droudnitsky
Copy link
Contributor

@droudnitsky droudnitsky commented Jun 18, 2024 •
edited
Loading

@Apache-HBase

This comment has been minimized.

Sign in to view
@Apache-HBase

This comment has been minimized.

Sign in to view
@Apache-HBase

This comment has been minimized.

Sign in to view
@Apache-HBase

This comment has been minimized.

Sign in to view
@droudnitsky
Copy link
Contributor Author

fixed spotless check

@Apache-HBase
Copy link

🎊 +1 overall

Vote Subsystem Runtime Comment
+0 🆗 reexec 0m 26s Docker mode activated.
-0 ⚠️ yetus 0m 2s Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck
_ Prechecks _
_ master Compile Tests _
+1 💚 mvninstall 2m 22s master passed
+1 💚 compile 0m 16s master passed
+1 💚 shadedjars 5m 40s branch has no errors when building our shaded downstream artifacts.
+1 💚 javadoc 0m 14s master passed
_ Patch Compile Tests _
+1 💚 mvninstall 2m 27s the patch passed
+1 💚 compile 0m 17s the patch passed
+1 💚 javac 0m 17s the patch passed
+1 💚 shadedjars 5m 42s patch has no errors when building our shaded downstream artifacts.
+1 💚 javadoc 0m 14s the patch passed
_ Other Tests _
+1 💚 unit 1m 26s hbase-client in the patch passed.
20m 2s
Subsystem Report/Notes
Docker ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6000/2/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile
GITHUB PR #6000
Optional Tests javac javadoc unit shadedjars compile
uname Linux 3363826a38ae 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 tool maven
Personality dev-support/hbase-personality.sh
git revision master / cd4c5c3
Default Java Temurin-1.8.0_352-b08
Test Results https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6000/2/testReport/
Max. process+thread count 301 (vs. ulimit of 30000)
modules C: hbase-client U: hbase-client
Console output https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6000/2/console
versions git=2.34.1 maven=3.8.6
Powered by Apache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

@Apache-HBase
Copy link

🎊 +1 overall

Vote Subsystem Runtime Comment
+0 🆗 reexec 0m 13s Docker mode activated.
-0 ⚠️ yetus 0m 3s Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck
_ Prechecks _
_ master Compile Tests _
+1 💚 mvninstall 2m 45s master passed
+1 💚 compile 0m 23s master passed
+1 💚 shadedjars 5m 11s branch has no errors when building our shaded downstream artifacts.
+1 💚 javadoc 0m 18s master passed
_ Patch Compile Tests _
+1 💚 mvninstall 2m 48s the patch passed
+1 💚 compile 0m 22s the patch passed
+1 💚 javac 0m 22s the patch passed
+1 💚 shadedjars 5m 11s patch has no errors when building our shaded downstream artifacts.
+1 💚 javadoc 0m 18s the patch passed
_ Other Tests _
+1 💚 unit 1m 44s hbase-client in the patch passed.
20m 5s
Subsystem Report/Notes
Docker ClientAPI=1.45 ServerAPI=1.45 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6000/2/artifact/yetus-jdk17-hadoop3-check/output/Dockerfile
GITHUB PR #6000
Optional Tests javac javadoc unit shadedjars compile
uname Linux faf05cec2766 5.4.0-182-generic #202-Ubuntu SMP Fri Apr 26 12:29:36 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality dev-support/hbase-personality.sh
git revision master / cd4c5c3
Default Java Eclipse Adoptium-17.0.10+7
Test Results https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6000/2/testReport/
Max. process+thread count 311 (vs. ulimit of 30000)
modules C: hbase-client U: hbase-client
Console output https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6000/2/console
versions git=2.34.1 maven=3.8.6
Powered by Apache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

@Apache-HBase
Copy link

🎊 +1 overall

Vote Subsystem Runtime Comment
+0 🆗 reexec 0m 39s Docker mode activated.
-0 ⚠️ yetus 0m 4s Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck
_ Prechecks _
_ master Compile Tests _
+1 💚 mvninstall 2m 45s master passed
+1 💚 compile 0m 20s master passed
+1 💚 shadedjars 5m 18s branch has no errors when building our shaded downstream artifacts.
+1 💚 javadoc 0m 17s master passed
_ Patch Compile Tests _
+1 💚 mvninstall 2m 46s the patch passed
+1 💚 compile 0m 20s the patch passed
+1 💚 javac 0m 20s the patch passed
+1 💚 shadedjars 5m 19s patch has no errors when building our shaded downstream artifacts.
+1 💚 javadoc 0m 17s the patch passed
_ Other Tests _
+1 💚 unit 1m 38s hbase-client in the patch passed.
20m 42s
Subsystem Report/Notes
Docker ClientAPI=1.45 ServerAPI=1.45 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6000/2/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile
GITHUB PR #6000
Optional Tests javac javadoc unit shadedjars compile
uname Linux 7a73ef98eb37 5.4.0-174-generic #193-Ubuntu SMP Thu Mar 7 14:29:28 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality dev-support/hbase-personality.sh
git revision master / cd4c5c3
Default Java Eclipse Adoptium-11.0.17+8
Test Results https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6000/2/testReport/
Max. process+thread count 287 (vs. ulimit of 30000)
modules C: hbase-client U: hbase-client
Console output https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6000/2/console
versions git=2.34.1 maven=3.8.6
Powered by Apache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

@Apache-HBase
Copy link

🎊 +1 overall

Vote Subsystem Runtime Comment
+0 🆗 reexec 0m 23s Docker mode activated.
_ Prechecks _
+1 💚 dupname 0m 0s No case conflicting files found.
+1 💚 hbaseanti 0m 0s Patch does not have any anti-patterns.
+1 💚 @author 0m 0s The patch does not contain any @author tags.
_ master Compile Tests _
+1 💚 mvninstall 3m 15s master passed
+1 💚 compile 0m 40s master passed
+1 💚 checkstyle 0m 15s master passed
+1 💚 spotless 0m 45s branch has no errors when running spotless:check.
+1 💚 spotbugs 0m 45s master passed
_ Patch Compile Tests _
+1 💚 mvninstall 2m 58s the patch passed
+1 💚 compile 0m 39s the patch passed
+1 💚 javac 0m 39s the patch passed
+1 💚 checkstyle 0m 14s the patch passed
+1 💚 whitespace 0m 0s The patch has no whitespace issues.
+1 💚 hadoopcheck 5m 38s Patch does not cause any errors with Hadoop 3.3.6.
+1 💚 spotless 0m 42s patch has no errors when running spotless:check.
+1 💚 spotbugs 0m 48s the patch passed
_ Other Tests _
+1 💚 asflicense 0m 10s The patch does not generate ASF License warnings.
24m 3s
Subsystem Report/Notes
Docker ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6000/2/artifact/yetus-general-check/output/Dockerfile
GITHUB PR #6000
Optional Tests dupname asflicense javac spotbugs hadoopcheck hbaseanti spotless checkstyle compile
uname Linux f0e229b4f262 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 tool maven
Personality dev-support/hbase-personality.sh
git revision master / cd4c5c3
Default Java Eclipse Adoptium-11.0.17+8
Max. process+thread count 78 (vs. ulimit of 30000)
modules C: hbase-client U: hbase-client
Console output https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6000/2/console
versions git=2.34.1 maven=3.8.6 spotbugs=4.7.3
Powered by Apache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

@Apache9
Copy link
Contributor

Apache9 commented Jun 22, 2024

For me this is reasonable. But anyway this is a behavior change, better start a discuss thread on the dev list to see if others have other opinions.

Thanks.

@droudnitsky
Copy link
Contributor Author

droudnitsky commented Jun 30, 2024 •
edited
Loading

Thank you for reviewing @Apache9 . I was originally under the impression that the default behavior was unintentional given the documentation in the hbase reference and the discussion in the work done in HBASE-24956 which added a new critical dependence on meta operation timeout in the locate region/meta lookup codepath in 2.x, but this may not have been unintentional as I originally thought. There seem to be two very different dependencies on meta operation timeout on 2.x:

  1. End to end operation timeout for system table operations (2.x and 3)
  2. Timeout to acquire userRegionLock to initiate meta scan in locateRegionInMeta (2.x blocking client only), this is the dependence on the meta operation timeout property that brought the 20 min default to my attention.

I am not familiar with how the locate region codepath on branch 3 works. But for branch 2 and the userRegionLock timeout, I don't think it makes sense to have default behavior be such that someone configuring client operation timeout can have operations taking many times longer than their client operation timeout due to contention around userRegionLock if they have not set the meta operation timeout.

I think it may be worth decoupling the userRegionLock timeout from the meta operation timeout, since we have one property that will dictate the behavior of two very different things on 2.x . I will put some more research and thought into this and perhaps raise a seperate issue to propose decoupling userRegionLock timeout from meta table operation timeout, which should make it simpler to reason about a default for meta operation timeout here. I'll start a dev list thread once I have gathered my thoughts on this, would be great to get more opinions on this, thank you for the suggestion and review.

@droudnitsky droudnitsky changed the title HBASE-28608 Correct client meta operation timeout to default to client operation timeout HBASE-28608 More sensible client meta operation timeout default Jun 30, 2024
@Apache9
Copy link
Contributor

Apache9 commented Jul 5, 2024

On branch-3+, we have removed all the sync client code and reimplement sync client on top of async client. In async client,the locate region timeout is controlled by meta operation timeout, but since we are asynchronous, the upper layer could get a timeout even if the locating region operation has not completed yet, so there is no problem.

On branch-2, it is a problem since we run all the operations in one thread.

Thanks.

@droudnitsky
Copy link
Contributor Author

droudnitsky commented Jul 13, 2024 •
edited
Loading

Thank you for the explanation @Apache9 . I have opened HBASE-28730 to propose decoupling the meta operation timeout from the branch-2 userRegionLock/locate region codepath. If my proposal there sounds reasonable, I can work to get that implemented, and once that big branch-2 userRegionLock dependence on the property is gone, it should be simpler to reason about the implications of a new default for meta operation timeout and I can bring this PR up on the devlist for discussion.

@droudnitsky
Copy link
Contributor Author

droudnitsky commented Oct 1, 2024 •
edited
Loading

Following up on my last comment, I originally was thinking to block this work on HBASE-28730 , but that issue itself is blocked on HBASE-27781 (which has patch available and is pending review), but even with HBASE-28730 completed there will still exist a dependency on meta operation timeout property , so I think would be good to get this in without blocking on HBASE-28730 + HBASE-27781, I will start a devlist discussion for the proposed change here shortly and see if there is support

@droudnitsky
Copy link
Contributor Author

Started dev list discussion here - https://lists.apache.org/thread/6q918prw2n6g90x1gfom4qf1hr3blxvv

@droudnitsky
Copy link
Contributor Author

Hi @Apache9 , from dev list thread started one week ago the proposal has received two +1s (so three +1s in total including your approval).

@Apache9 Apache9 merged commit beb36e8 into apache:master Nov 21, 2024
1 check passed
Apache9 pushed a commit that referenced this pull request Nov 21, 2024
@droudnitsky @Apache9
HBASE-28608 Correct client meta operation timeout to default to clien…
83698f7 
…t operation timeout (#6000)

Co-authored-by: Daniel Roudnitsky <droudnitsky1@bloomberg.net>
Signed-off-by: Duo Zhang <zhangduo@apache.org>
(cherry picked from commit beb36e8)
gvprathyusha6 pushed a commit to gvprathyusha6/hbase that referenced this pull request Dec 19, 2024
@droudnitsky
HBASE-28608 Correct client meta operation timeout to default to clien…
90b4699 
…t operation timeout (apache#6000)

Co-authored-by: Daniel Roudnitsky <droudnitsky1@bloomberg.net>
Signed-off-by: Duo Zhang zhangduo@apache.org
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Apache9 Apache9 Apache9 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@droudnitsky @Apache-HBase @Apache9