[GitHub] [hbase-native-client] bharathv commented on a change in pull request #7: HBASE-24538: Simplify the lifecycle of mini cluster

[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