You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hbase.apache.org by GitBox <gi...@apache.org> on 2020/06/15 05:45:12 UTC
[GitHub] [hbase-native-client] bharathv opened a new pull request #7: HBASE-24538: Simplify the lifecycle of mini cluster
bharathv opened a new pull request #7:
URL: https://github.com/apache/hbase-native-client/pull/7
Cleans up unnecessary methods and moves the teardown of the mini cluster
to the d'tor so that an explicit cleanup is not needed. It is not prone
to the deadlocks mentioned in the jira anymore.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase-native-client] bharathv merged pull request #7: HBASE-24538: Simplify the lifecycle of mini cluster
Posted by GitBox <gi...@apache.org>.
bharathv merged pull request #7:
URL: https://github.com/apache/hbase-native-client/pull/7
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase-native-client] phrocker commented on a change in pull request #7: HBASE-24538: Simplify the lifecycle of mini cluster
Posted by GitBox <gi...@apache.org>.
phrocker commented on a change in pull request #7:
URL: https://github.com/apache/hbase-native-client/pull/7#discussion_r440118820
##########
File path: src/hbase/test-util/mini-cluster.cc
##########
@@ -93,7 +97,8 @@ MiniCluster::~MiniCluster() {
}
void MiniCluster::Setup() {
Review comment:
I think MiniCluster's usage should be declarative. If it is instantiated it will be used. As it stands, the JVM creation and assignment is performed lazily, but as you pointed out, it creates a state management issue. If we declare a MiniCluster I think we should create the JVM and class/method objects in the constructor. This will allow the destructor to destroy the java MiniCluster. Is there a case where MiniCluster is created without the Java based mini cluster is used?
I may be missing something..but it seems that the code would be dramatically simplified if Setup is always performed in the constructor and StopCluster is always performed in the destructor. Then the caller would be responsible for calling methods that would start/stop the cluster.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase-native-client] bharathv commented on pull request #7: HBASE-24538: Simplify the lifecycle of mini cluster
Posted by GitBox <gi...@apache.org>.
bharathv commented on pull request #7:
URL: https://github.com/apache/hbase-native-client/pull/7#issuecomment-645563707
Thanks for the review @phrocker.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase-native-client] bharathv commented on pull request #7: HBASE-24538: Simplify the lifecycle of mini cluster
Posted by GitBox <gi...@apache.org>.
bharathv commented on pull request #7:
URL: https://github.com/apache/hbase-native-client/pull/7#issuecomment-643915442
@joshelser @phrocker FYI
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase-native-client] phrocker commented on a change in pull request #7: HBASE-24538: Simplify the lifecycle of mini cluster
Posted by GitBox <gi...@apache.org>.
phrocker commented on a change in pull request #7:
URL: https://github.com/apache/hbase-native-client/pull/7#discussion_r441462484
##########
File path: src/hbase/test-util/mini-cluster.cc
##########
@@ -306,3 +289,14 @@ jclass hbase::MiniCluster::FindClassAndGetGlobalRef(const char *cls_name) {
env_->DeleteLocalRef(local_ref);
return static_cast<jclass>(global_ref);
}
+
+void hbase::MiniCluster::StopCluster() {
+ CHECK_NOTNULL(env_);
+ lock_guard<mutex> lock(lock_);
Review comment:
Sorry for the delay. I've given this some thought and agree with what you are asserting. It would hinder the flow of logic and/or require odd levels of scoping in that type of sequence.
Scope guards might help as would a singleton around the initialization of the JVM; however, I think what is here is fine. I don't think you should re-do it solves the issue. Thanks!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase-native-client] phrocker commented on a change in pull request #7: HBASE-24538: Simplify the lifecycle of mini cluster
Posted by GitBox <gi...@apache.org>.
phrocker commented on a change in pull request #7:
URL: https://github.com/apache/hbase-native-client/pull/7#discussion_r440118820
##########
File path: src/hbase/test-util/mini-cluster.cc
##########
@@ -93,7 +97,8 @@ MiniCluster::~MiniCluster() {
}
void MiniCluster::Setup() {
Review comment:
I think MiniCluster's usage should be declarative. If it is instantiated it will be used. As it stands, the JVM creation and assignment is performed lazily, but as you pointed out, it creates a state management issue. If we declare a MiniCluster I think we should create the JVM and class/method objects in the constructor. This will allow the destructor to destroy the java MiniCluster. Is there a case where MiniCluster is created without the Java based mini cluster being used?
I may be missing something..but it seems that the code would be simplified if Setup is always performed in the constructor and StopCluster is always performed in the destructor. I would assume that stopMiniCluster can be called in Java no matter the state of the mini cluster.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase-native-client] phrocker commented on a change in pull request #7: HBASE-24538: Simplify the lifecycle of mini cluster
Posted by GitBox <gi...@apache.org>.
phrocker commented on a change in pull request #7:
URL: https://github.com/apache/hbase-native-client/pull/7#discussion_r440115909
##########
File path: src/hbase/test-util/mini-cluster.cc
##########
@@ -306,3 +289,14 @@ jclass hbase::MiniCluster::FindClassAndGetGlobalRef(const char *cls_name) {
env_->DeleteLocalRef(local_ref);
return static_cast<jclass>(global_ref);
}
+
+void hbase::MiniCluster::StopCluster() {
+ CHECK_NOTNULL(env_);
+ lock_guard<mutex> lock(lock_);
Review comment:
Unless I'm missing something ( usually am ), If we move assignment of testing_util_class to the constructor we shouldn't need a mutex and we shouldn't need to check if htu exists. This would allow StopCluster to be used in the destructor safely.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase-native-client] bharathv commented on a change in pull request #7: HBASE-24538: Simplify the lifecycle of mini cluster
Posted by GitBox <gi...@apache.org>.
bharathv commented on a change in pull request #7:
URL: https://github.com/apache/hbase-native-client/pull/7#discussion_r440358850
##########
File path: src/hbase/test-util/mini-cluster.cc
##########
@@ -306,3 +289,14 @@ jclass hbase::MiniCluster::FindClassAndGetGlobalRef(const char *cls_name) {
env_->DeleteLocalRef(local_ref);
return static_cast<jclass>(global_ref);
}
+
+void hbase::MiniCluster::StopCluster() {
+ CHECK_NOTNULL(env_);
+ lock_guard<mutex> lock(lock_);
Review comment:
Ya, I actually thought about this, the issue is that the cluster operations could be a little more complex than just start/stop of cluster. A typical sequence (in the future tests) could be like
StartCluster()
StopCluster()
StartCluster() ...again
AddRegionServer()
StopRegionServer()
Stop()
.....
So typically all of them need to grab this mutex before making the change, otherwise we'd probably end up in a weird state (for ex: destroying the JVM while say addRegionServer() etc.)... So I retained the mutex as is rather than moving it into the c'tor and d'tor. What do you think?
##########
File path: src/hbase/test-util/mini-cluster.cc
##########
@@ -93,7 +97,8 @@ MiniCluster::~MiniCluster() {
}
void MiniCluster::Setup() {
Review comment:
I think this ties to my response to the other comment. My understanding is that the cluster lifecycle could potentially be a little more complex than just start/stop, hence the mutex. Let me know if you think its an overkill and I can redo the patch. I agree with you that it simplifies the overall patch.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org