[
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)
