hbase-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Enis Soztutar (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HBASE-17680) Run mini cluster through JNI in tests
Date Thu, 02 Mar 2017 23:22:45 GMT

    [ https://issues.apache.org/jira/browse/HBASE-17680?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15893228#comment-15893228
] 

Enis Soztutar commented on HBASE-17680:
---------------------------------------

I've checked the v23 patch. You did not address these previous comments: 
- There is still a very large number of these kind of warnings: 
{code}
core/filter-test.cc:65:39: warning: ISO C++11 does not allow conversion from string literal
to 'char *' [-Wwritable-strings]
  FilterTest::test_util_->CreateTable("t", "d");
{code}
You should use std::string everywhere, except when you are sending it to the JVM object. So,
the method signatures like: 
{code}
jobject MiniCluster::CreateTable(char *tblNam, char *familyName) {
{code} 
should be always taking std::string, or better {{const std::string&}}. We want to minimize
the {{char *}} usage to be only happening in the JVM layer. You can use string::c_str(). 
- Also this warning: 
{code}
test-util/mini-cluster.cc:260:1: warning: control reaches end of non-void function [-Wreturn-type]
{code}
- Some of the fields in MiniCluster does not end with {{_}}. Examples are {{cluster}}, {{jvm}},
etc. 
- As per above, you should not need WriteConf() method at all. Please construct a Configuration
object and populate it via calling {{Configuration::Set()}} from the java-level Configuration
object. You seem to be doing that in TestUtil, so please remove the WriteConf and setting
the path, etc. Also return std::string in GetConf(), etc. 
- We use {{make lint}} to run the cpplint script. Can you please address these new warnings
as well: 
{code}
test-util/test-util.cc:65:  Add #include <memory> for make_unique<>  [build/include_what_you_use]
[4]
test-util/test-util.cc:77:  Could not find a newline character at the end of the file.  [whitespace/ending_newline]
[5]
Done processing test-util/test-util.cc
test-util/test-util.h:84:  Add #include <memory> for shared_ptr<>  [build/include_what_you_use]
[4]
Done processing test-util/test-util.h
test-util/mini-cluster.cc:22:  "glog/logging.h" already included at test-util/mini-cluster.cc:21
 [build/include] [4]
test-util/mini-cluster.cc:25:  Found C system header after C++ system header. Should be: mini-cluster.h,
c system, c++ system, other.  [build/include_order] [4]
test-util/mini-cluster.cc:26:  Found C system header after C++ system header. Should be: mini-cluster.h,
c system, c++ system, other.  [build/include_order] [4]
test-util/mini-cluster.cc:70:  Missing space before {  [whitespace/braces] [5]
test-util/mini-cluster.cc:75:  Using C-style cast.  Use reinterpret_cast<void **>(...)
instead  [readability/casting] [4]
test-util/mini-cluster.cc:227:  Using C-style cast.  Use reinterpret_cast<jbyte *>(...)
instead  [readability/casting] [4]
test-util/mini-cluster.cc:313:  Add #include <string> for string  [build/include_what_you_use]
[4]
Done processing test-util/mini-cluster.cc
test-util/mini-cluster.h:28:  Include the directory when naming .h files  [build/include]
[4]
test-util/mini-cluster.h:35:  Add #include <string> for string  [build/include_what_you_use]
[4]
{code}
- GetConf() is overloaded. For the one returning a configuration value, you should name it
GetConfValue(). 
- Should the jobject cluster be a field in MiniCluster. No need to pass it to methods like
GetConf() I think. In StartCluster(), you can save it in the field. 
- This behavior coming from the existing code: 
{code}
Creating a TestUtil will spin up a cluster with numRegionServers region servers.
{code}
But I think it is wrong. Can we change it so that TestUtil ctor does not start the cluster,
but you have to manually call StartCluster. I think it is better this way, because in theory
you should be able use the test util without the cluster. 
Other then these, the patch looks good. 


> Run mini cluster through JNI in tests
> -------------------------------------
>
>                 Key: HBASE-17680
>                 URL: https://issues.apache.org/jira/browse/HBASE-17680
>             Project: HBase
>          Issue Type: Sub-task
>            Reporter: Ted Yu
>            Assignee: Ted Yu
>         Attachments: 17680.v14.txt, 17680.v17.txt, 17680.v18.txt, 17680.v1.txt, 17680.v20.txt,
17680.v22.txt, 17680.v23.txt, 17680.v3.txt, 17680.v8.txt
>
>
> Currently tests start local hbase cluster through hbase shell.
> There is less control over the configuration of the local cluster this way.
> This issue would replace hbase shell with JNI interface to mini cluster.
> We would have full control over the cluster behavior.
> Thanks to [~devaraj] who started this initiative.



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

Mime
View raw message