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:
[email protected]
