Skip to content

Conversation

@briaugenreich
Copy link

@briaugenreich briaugenreich commented Aug 17, 2022

mostly opening here to get some feedback before opening upstream pr.

https://issues.apache.org/jira/browse/HBASE-26809

Copy link
Author

@briaugenreich briaugenreich left a comment

Choose a reason for hiding this comment

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

a few things i wanted to clarify before testing + adding the last metric location

@briaugenreich
Copy link
Author

briaugenreich commented Aug 18, 2022

Updated to produce metrics only during times of server overload. Now I have two remaining questions.

  1. What should the scope of testing look like for this? I ran the following successfully, but I have not made any modifications to existing tests. Does this warrant simulating an overload and verifying these metrics are shipped? Im thinking it would go somewhere in the testRetryPauseWhenServerIsOverloaded in the TestAsyncProcess tests.
 mvn clean verify -pl hbase-client -am -Phadoop-3.0 -Dhadoop.profile=3.0  -Dtest=TestMetricsConnection,TestAsyncProcess 


[INFO] -------------------------------------------------------
[INFO]  T E S T S
[INFO] -------------------------------------------------------
[INFO] Running org.apache.hadoop.hbase.client.TestAsyncProcess
[INFO] Running org.apache.hadoop.hbase.client.TestMetricsConnection
[INFO] Tests run: 3, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 1.377 s - in org.apache.hadoop.hbase.client.TestMetricsConnection
[INFO] Tests run: 41, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 83.055 s - in org.apache.hadoop.hbase.client.TestAsyncProcess
.
.
.
[INFO] -------------------------------------------------------
[INFO]  T E S T S
[INFO] -------------------------------------------------------
[INFO] Running org.apache.hadoop.hbase.client.TestMetricsConnection
[INFO] Tests run: 3, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 1.237 s - in org.apache.hadoop.hbase.client.TestMetricsConnection
[INFO] 
.
.
.
[INFO] Apache HBase - Client .............................. SUCCESS [ 12.135 s]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  01:20 min
[INFO] Finished at: 2022-08-18T16:12:02-04:00
[INFO] ------------------------------------------------------------------------


  1. The RpcRetryingCallerImpl doesn't seem to have a way to access the connection and therefore the metrics connection without changes to the method contracts. From what I've gathered RpcRetryingCallerImpl is given a RetryingCallable to invoke. Within RetryingCallable.prepare() is where you might see the connection setup etc. Would it make sense to add something to the RetryingCallable interface? Though that looks like it has 143 implementations soo may get a little crazy?

@briaugenreich
Copy link
Author

Okay... updated to resolve question #2 I posted above and ran the following tests...

 mvn clean verify -pl hbase-client -am -Phadoop-3.0 -Dhadoop.profile=3.0  -Dtest=TestMetricsConnection,TestAsyncProcess,TestAsyncProcessWithRegionException,TestRpcRetryingCallerImpl,TestHBaseAdminNoCluster 

[INFO] --- maven-surefire-plugin:3.0.0-M4:test (default-test) @ hbase-client ---
[INFO] 
[INFO] -------------------------------------------------------
[INFO]  T E S T S
[INFO] -------------------------------------------------------
[INFO] Running org.apache.hadoop.hbase.client.TestAsyncProcess
[INFO] Running org.apache.hadoop.hbase.client.TestMetricsConnection
[INFO] Running org.apache.hadoop.hbase.client.TestRpcRetryingCallerImpl
[INFO] Tests run: 2, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 1.031 s - in org.apache.hadoop.hbase.client.TestRpcRetryingCallerImpl
[INFO] Running org.apache.hadoop.hbase.client.TestAsyncProcessWithRegionException
[INFO] Tests run: 3, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 1.975 s - in org.apache.hadoop.hbase.client.TestMetricsConnection
[INFO] Tests run: 3, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 5.335 s - in org.apache.hadoop.hbase.client.TestAsyncProcessWithRegionException
[INFO] Tests run: 41, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 87.583 s - in org.apache.hadoop.hbase.client.TestAsyncProcess

The only remaining item I'm looking into is if I need to make additional changes to some of the other instantiate methods.

b74747c#diff-1c9b1c63630f33e323daf8f874758e9efc5ec6ed4a40f2456db4bbd82209cac4R102

@briaugenreich
Copy link
Author

@bbeaudreault should this go in a particular 2.0 version or can I just open on branch-2?

@bbeaudreault
Copy link
Member

bbeaudreault commented Sep 13, 2022

Sorry i just saw this -- Let's close this PR and you can open a new PR against branch-2.

Generally we PR against master first (done), then it might just get cherry-picked to the rest (didn't work here), so then we do branch-2, then it might get cherry-picked to the rest (probably will work, we'll see), then we do others like branch-2.5, etc depending on how far back we want to backport.

@briaugenreich
Copy link
Author

Got it, thanks! yeah the cherry pick against branch 2 has so many merge conflicts I think I'm going to have to manually apply these.

@briaugenreich briaugenreich deleted the HBASE-26809_hubspot-2 branch December 1, 2022 14:35
Sign up for free to 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.

3 participants