HBASE-24805 HBaseTestingUtility.getConnection should be threadsafe #2180
Conversation
| }; | ||
|
|
||
| protected Configuration conf; | ||
| protected final Configuration conf; |
There was a problem hiding this comment.
Changing this variable to final breaks binary compatibility, but is needed to make access to this field threadsafe.
I could maintain binary compatibility by making a new final variable called "originalConf" or something like that which is used by our own internals. but that would still cause a behavior change because if someone made a subclass of these classes that assigned to the conf variable we would not pay any attention to that change.
I think we should just release note the break, especially given the on-going issue of "should we be labeling these things IA.Public yet?"
There was a problem hiding this comment.
I think we should just release note the break, especially given the on-going issue of "should we be labeling these things IA.Public yet?"
Agreed with putting description on the release notes. And feel weird that these are IA Public, should it be changed to IA Private here?
There was a problem hiding this comment.
should it be changed to IA Private here?
no, there 's a whole back and forth community discussion that has been on-going in bits and pieces for years. the short version is that we definitely need something downstream facing for running tests of hbase client code, but no one has provided a clean way to separate that out from how we test our own stuff.
There was a problem hiding this comment.
s/hbase client code/code that uses hbase client
There was a problem hiding this comment.
Since we are anyways going to release note the breaking change, can we also make this field private? Downstream extended classes will have to mandatorily start using super constructor for setting conf anyways and now that it is final, we can also make it private.
There was a problem hiding this comment.
we make use of this across our internal subclasses of HBaseCommonTestingUtility so if we made it private instead of protected we'd need to add an accessor.
wchevreuil
left a comment
wchevreuil
left a comment
There was a problem hiding this comment.
Approving, assuming pre-commit builds are all ok.
| }; | ||
|
|
||
| protected Configuration conf; | ||
| protected final Configuration conf; |
There was a problem hiding this comment.
I think we should just release note the break, especially given the on-going issue of "should we be labeling these things IA.Public yet?"
Agreed with putting description on the release notes. And feel weird that these are IA Public, should it be changed to IA Private here?
virajjasani
left a comment
virajjasani
left a comment
There was a problem hiding this comment.
+1, pending QA (tests should not see any issues hopefully)
Thank you
|
🎊 +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. |
|
🎊 +1 overall
This message was automatically generated. |
|
I do not know why the precommit job on ci-hadoop ran multiple jobs for this PR, but in run number 2 this was the failure: in run number three there were no failures. Looking at the test ouput for TestFromClientSideWithCoprocessor5, I don't think there's enough detail to figure out what happened. It looks like the final two were loss of ZK under load: If I had to guess I'd say in the first case maybe we have a retry mechanism that can recount scan metrics? In any case I am reasonably certain these failures are not related to the test change here. |
* refactor how we use connection to rely on the access method * refactor initialization and cleanup of the shared connection * incompatibly change HCTU's Configuration member variable to be final so it can be safely accessed from multiple threads. Closes #2180 Signed-off-by: Wellington Chevreuil <[email protected]> Signed-off-by: Viraj Jasani <[email protected]> (cherry picked from commit 86ebbdd)
@busbey this was my doing, I did it for the same reason on test failure i.e "could not figure out if the test failure was relevant as per stdout and stderr" and hence thought of triggering another build. |
* refactor how we use connection to rely on the access method * refactor initialization and cleanup of the shared connection * incompatibly change HCTU's Configuration member variable to be final so it can be safely accessed from multiple threads. Closes apache#2180 adapted for jdk7 (cherry picked from commit 86ebbdd) (cherry picked from commit 0806349)
* refactor how we use connection to rely on the access method * refactor initialization and cleanup of the shared connection * incompatibly change HCTU's Configuration member variable to be final so it can be safely accessed from multiple threads. Closes apache#2180 Signed-off-by: Wellington Chevreuil <[email protected]> Signed-off-by: Viraj Jasani <[email protected]> (cherry picked from commit 86ebbdd)
4 participants
This maintains most compatibility while changing the handling of the shared connection to threadsafe. Note about compat as comments.