You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Grant Henke (Code Review)" <ge...@cloudera.org> on 2018/09/19 19:33:06 UTC
[kudu-CR] [test] Clean up MiniKuduCluster and BaseKuduTest
Grant Henke has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11474
Change subject: [test] Clean up MiniKuduCluster and BaseKuduTest
......................................................................
[test] Clean up MiniKuduCluster and BaseKuduTest
This patch cleans up the MiniKuduCluster and
BaseKuduTest API in preparation for creating a
public test API and improving per-test
MiniKuduCluster parameters.
This patch also removes the use of HostAndPort as
part of the clean up. It replaces it with unresolved InetSocketAddress because HostAndPort is provided
by Guava which is shaded and relocated in all of our
libraries. This means it shouldn’t be a part of the
“public” API. In this case that means the test API
too. Additionally HostAndPort is marked as beta.
Change-Id: I2a55c41f48f31c4a12b41508dbb343c4419e65b1
---
M java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala
M java/kudu-client-tools/src/test/java/org/apache/kudu/mapreduce/tools/ITExportCsv.java
M java/kudu-client-tools/src/test/java/org/apache/kudu/mapreduce/tools/ITImportCsv.java
M java/kudu-client-tools/src/test/java/org/apache/kudu/mapreduce/tools/ITImportParquet.java
M java/kudu-client-tools/src/test/java/org/apache/kudu/mapreduce/tools/ITImportParquetPreCheck.java
M java/kudu-client-tools/src/test/java/org/apache/kudu/mapreduce/tools/ITIntegrationTestBigLinkedList.java
M java/kudu-client-tools/src/test/java/org/apache/kudu/mapreduce/tools/ITRowCounter.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToCluster.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToClusterResponse.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java
M java/kudu-client/src/main/java/org/apache/kudu/util/NetUtil.java
M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java
M java/kudu-client/src/test/java/org/apache/kudu/client/ITClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/ITClientStress.java
M java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduSession.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestClientFailoverSupport.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectToCluster.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectionCache.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestHandleTooBusy.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestMasterFailover.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestMiniKuduCluster.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestMultipleLeaderFailover.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestNegotiation.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestRemoteTablet.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurity.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurityContextRealUser.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestServerInfo.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestTimeouts.java
M java/kudu-client/src/test/java/org/apache/kudu/util/TestNetUtil.java
M java/kudu-flume-sink/src/test/java/org/apache/kudu/flume/sink/AvroKuduOperationsProducerTest.java
M java/kudu-flume-sink/src/test/java/org/apache/kudu/flume/sink/KeyedKuduOperationsProducerTest.java
M java/kudu-flume-sink/src/test/java/org/apache/kudu/flume/sink/KuduSinkTest.java
M java/kudu-flume-sink/src/test/java/org/apache/kudu/flume/sink/RegexpKuduOperationsProducerTest.java
M java/kudu-mapreduce/src/test/java/org/apache/kudu/mapreduce/ITInputFormatJob.java
M java/kudu-mapreduce/src/test/java/org/apache/kudu/mapreduce/ITKuduTableInputFormat.java
M java/kudu-mapreduce/src/test/java/org/apache/kudu/mapreduce/ITKuduTableOutputFormat.java
M java/kudu-mapreduce/src/test/java/org/apache/kudu/mapreduce/ITOutputFormatJob.java
M java/kudu-spark-tools/src/test/scala/org/apache/kudu/spark/tools/ITBigLinkedListTest.scala
M java/kudu-spark-tools/src/test/scala/org/apache/kudu/spark/tools/TestImportExportFiles.scala
M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/DefaultSourceTest.scala
M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduContextTest.scala
M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduTestSuite.scala
47 files changed, 451 insertions(+), 378 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/11474/1
--
To view, visit http://gerrit.cloudera.org:8080/11474
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I2a55c41f48f31c4a12b41508dbb343c4419e65b1
Gerrit-Change-Number: 11474
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <gr...@apache.org>
[kudu-CR] [test] Clean up MiniKuduCluster and BaseKuduTest
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11474 )
Change subject: [test] Clean up MiniKuduCluster and BaseKuduTest
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11474/1/java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java
File java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java:
http://gerrit.cloudera.org:8080/#/c/11474/1/java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java@77
PS1, Line 77: getHostString
> I think both of those are options. I don't want to roll all the parsing and
OK, do you intend to apply one of these (or something similar) to this patch?
--
To view, visit http://gerrit.cloudera.org:8080/11474
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2a55c41f48f31c4a12b41508dbb343c4419e65b1
Gerrit-Change-Number: 11474
Gerrit-PatchSet: 3
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Thu, 20 Sep 2018 19:28:00 +0000
Gerrit-HasComments: Yes
[kudu-CR] [test] Clean up MiniKuduCluster and BaseKuduTest
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11474
to look at the new patch set (#2).
Change subject: [test] Clean up MiniKuduCluster and BaseKuduTest
......................................................................
[test] Clean up MiniKuduCluster and BaseKuduTest
This patch cleans up the MiniKuduCluster and
BaseKuduTest API in preparation for creating a
public test API and improving per-test
MiniKuduCluster parameters.
This patch also removes the use of HostAndPort as
part of the clean up. It replaces it with unresolved
InetSocketAddress because HostAndPort is provided
by Guava which is shaded and relocated in all of our
libraries. This means it shouldn’t be a part of the
“public” API. In this case that means the test API
too. Additionally HostAndPort is marked as beta.
Change-Id: I2a55c41f48f31c4a12b41508dbb343c4419e65b1
---
M java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala
M java/kudu-client-tools/src/test/java/org/apache/kudu/mapreduce/tools/ITExportCsv.java
M java/kudu-client-tools/src/test/java/org/apache/kudu/mapreduce/tools/ITImportCsv.java
M java/kudu-client-tools/src/test/java/org/apache/kudu/mapreduce/tools/ITImportParquet.java
M java/kudu-client-tools/src/test/java/org/apache/kudu/mapreduce/tools/ITImportParquetPreCheck.java
M java/kudu-client-tools/src/test/java/org/apache/kudu/mapreduce/tools/ITIntegrationTestBigLinkedList.java
M java/kudu-client-tools/src/test/java/org/apache/kudu/mapreduce/tools/ITRowCounter.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToCluster.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToClusterResponse.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java
M java/kudu-client/src/main/java/org/apache/kudu/util/NetUtil.java
M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java
M java/kudu-client/src/test/java/org/apache/kudu/client/ITClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/ITClientStress.java
M java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduSession.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestClientFailoverSupport.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectToCluster.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectionCache.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestHandleTooBusy.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestMasterFailover.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestMiniKuduCluster.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestMultipleLeaderFailover.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestNegotiation.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestRemoteTablet.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurity.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurityContextRealUser.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestServerInfo.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestTimeouts.java
M java/kudu-client/src/test/java/org/apache/kudu/util/TestNetUtil.java
M java/kudu-flume-sink/src/test/java/org/apache/kudu/flume/sink/AvroKuduOperationsProducerTest.java
M java/kudu-flume-sink/src/test/java/org/apache/kudu/flume/sink/KeyedKuduOperationsProducerTest.java
M java/kudu-flume-sink/src/test/java/org/apache/kudu/flume/sink/KuduSinkTest.java
M java/kudu-flume-sink/src/test/java/org/apache/kudu/flume/sink/RegexpKuduOperationsProducerTest.java
M java/kudu-mapreduce/src/test/java/org/apache/kudu/mapreduce/ITInputFormatJob.java
M java/kudu-mapreduce/src/test/java/org/apache/kudu/mapreduce/ITKuduTableInputFormat.java
M java/kudu-mapreduce/src/test/java/org/apache/kudu/mapreduce/ITKuduTableOutputFormat.java
M java/kudu-mapreduce/src/test/java/org/apache/kudu/mapreduce/ITOutputFormatJob.java
M java/kudu-spark-tools/src/test/scala/org/apache/kudu/spark/tools/ITBigLinkedListTest.scala
M java/kudu-spark-tools/src/test/scala/org/apache/kudu/spark/tools/TestImportExportFiles.scala
M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/DefaultSourceTest.scala
M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduContextTest.scala
M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduTestSuite.scala
47 files changed, 452 insertions(+), 378 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/11474/2
--
To view, visit http://gerrit.cloudera.org:8080/11474
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2a55c41f48f31c4a12b41508dbb343c4419e65b1
Gerrit-Change-Number: 11474
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] [test] Clean up MiniKuduCluster and BaseKuduTest
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/11474 )
Change subject: [test] Clean up MiniKuduCluster and BaseKuduTest
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/11474/1/java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java
File java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java:
http://gerrit.cloudera.org:8080/#/c/11474/1/java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java@77
PS1, Line 77: getHostName()
> More broadly, this is the main issue I have with InetSocketAddress: it's ea
I think both of those are options. I don't want to roll all the parsing and validation logic. I suppose we could make our own HostAndPort that holds and instance of InetSocketAddress internally.
The main reason we can't use HostAndPort is because we shade and relocate Guava. This means that it should not be a parameter or return value for any "public" methods. Especially if they are to be used across modules.
http://gerrit.cloudera.org:8080/#/c/11474/1/java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
File java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java:
http://gerrit.cloudera.org:8080/#/c/11474/1/java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java@178
PS1, Line 178: throw new RuntimeException(resp.getError().getMessage());
> I guess I don't understand the dependency issue; are you saying MiniKuduClu
Yeah, that is what I am saying. It will take some more changes to make that work. Like you pointed out SecurityUtils and Protobufs.
I am talking about follow on work outside of this patch where MiniKuduCluster is in it's own module. If that module needed kudu-client then we couldn't use it in the kudu-client tests. It's a dependency ordering problem. I think Gradle could handle it, but not Maven. Even still it's worth keeping them separate.
I think it would be easier to explain outside of chat, or show with a sample patch. I am working on those patches.
--
To view, visit http://gerrit.cloudera.org:8080/11474
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2a55c41f48f31c4a12b41508dbb343c4419e65b1
Gerrit-Change-Number: 11474
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Wed, 19 Sep 2018 21:27:46 +0000
Gerrit-HasComments: Yes
[kudu-CR] [test] Clean up MiniKuduCluster and BaseKuduTest
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11474 )
Change subject: [test] Clean up MiniKuduCluster and BaseKuduTest
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/11474
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2a55c41f48f31c4a12b41508dbb343c4419e65b1
Gerrit-Change-Number: 11474
Gerrit-PatchSet: 5
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Fri, 21 Sep 2018 05:22:12 +0000
Gerrit-HasComments: No
[kudu-CR] [test] Clean up MiniKuduCluster and BaseKuduTest
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/11474 )
Change subject: [test] Clean up MiniKuduCluster and BaseKuduTest
......................................................................
Patch Set 1:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/11474/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11474/1//COMMIT_MSG@11
PS1, Line 11: public test API
> We're not committing to this with the same level of rigor or compatibility
We haven't committed to anything at all yet. I will clearly document our compatibility guarantees when the move to being actually public happens.
http://gerrit.cloudera.org:8080/#/c/11474/1//COMMIT_MSG@15
PS1, Line 15: part of the clean up. It replaces it with unresolved InetSocketAddress because HostAndPort is provided
> Nit: got a too long line here
Done
http://gerrit.cloudera.org:8080/#/c/11474/1/java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
File java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java:
http://gerrit.cloudera.org:8080/#/c/11474/1/java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java@345
PS1, Line 345: public static InetSocketAddress AddressFromPB(Common.HostPortPB hostPortPB) {
> Should be addressFromPB.
Done
http://gerrit.cloudera.org:8080/#/c/11474/1/java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java
File java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java:
http://gerrit.cloudera.org:8080/#/c/11474/1/java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java@77
PS1, Line 77: getHostName()
> Should we use getHostString() in place of getHostName()? The documentation
See my other answer for more detail. The tl;dr is that a lookup can only occur if the `InetSocketAddress` was created with `new InetSocketAddress(InetAddress addr, int port)`.
We can use getHostString() to be sure since the `InetAddress.getByName` call will handle any lookup needed.
http://gerrit.cloudera.org:8080/#/c/11474/1/java/kudu-client/src/main/java/org/apache/kudu/util/NetUtil.java
File java/kudu-client/src/main/java/org/apache/kudu/util/NetUtil.java:
http://gerrit.cloudera.org:8080/#/c/11474/1/java/kudu-client/src/main/java/org/apache/kudu/util/NetUtil.java@67
PS1, Line 67: return InetSocketAddress.createUnresolved(hostAndPort.getHost(), hostAndPort.getPort());
> How does the resulting InetSocketAddress behave when the 'host' argument is
`InetSocketAddress` will set 1.2.3.4 as the hostname. Methods like `getHostName` and `getHostString` will always return the supplied value.
The only time hostname lookups can occur is when the InetSocketAddress is created with `new InetSocketAddress(InetAddress addr, int port)`.
http://gerrit.cloudera.org:8080/#/c/11474/1/java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
File java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java:
http://gerrit.cloudera.org:8080/#/c/11474/1/java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java@178
PS1, Line 178: throw new RuntimeException(resp.getError().getMessage());
> Why this change?
This was a carry-over from my WIP "breakout MiniKuduCluster to it's own Module" class. NonRecoverableException is a KuduClient specific exception and I couldn't have the dependency.
Outside of that, wrapping in a status object and using a checked exception doesn't really add any value. It results in the same end behavior; a failed test because of an IOException with the error as the message.
I can use an IOException to be a bit closer to the NonRecoverableException.
http://gerrit.cloudera.org:8080/#/c/11474/1/java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java@297
PS1, Line 297: public void startMasterServer(InetSocketAddress address) throws IOException {
> FWIW, the reason these methods were prefixed with 'start' and not 'restart'
Makes sense. I don't think it's possible (currently) to have a server that has never been started before in our BaseKuduTest class. I think thats why I found "restart" confusing. It sounded like it would shutdown and start the server.
Perhaps I could enhance this to handle starting a server from scratch. Think that makes sense?
http://gerrit.cloudera.org:8080/#/c/11474/1/java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java@479
PS1, Line 479: /**
: * Converts HostPortPB to an unresolved InetSocketAddress.
: *
: * @param pb the host and port to convert
: * @return an unresolved InetSocketAddress
: */
: private static InetSocketAddress addressFromPB(HostPortPB pb) {
: return InetSocketAddress.createUnresolved(pb.getHost(), pb.getPort());
: }
> Can't reuse the version in ProtobufHelper? Is this more of the TODO(KUDU-21
This isn't KUDU-2186. In fact that issue is fixed by removing HostAndPort from the public APIs. That's what was causing shading issues.
This is another case of carryover from my WIP patch. I didn't want the MiniKuduCluster to have a dependency on the kudu-client.
--
To view, visit http://gerrit.cloudera.org:8080/11474
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2a55c41f48f31c4a12b41508dbb343c4419e65b1
Gerrit-Change-Number: 11474
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Wed, 19 Sep 2018 20:48:12 +0000
Gerrit-HasComments: Yes
[kudu-CR] [test] Clean up MiniKuduCluster and BaseKuduTest
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11474 )
Change subject: [test] Clean up MiniKuduCluster and BaseKuduTest
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/11474/1/java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java
File java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java:
http://gerrit.cloudera.org:8080/#/c/11474/1/java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java@77
PS1, Line 77: getHostString
> See my other answer for more detail. The tl;dr is that a lookup can only oc
More broadly, this is the main issue I have with InetSocketAddress: it's easy to accidentally introduce DNS lookups. Yes, you've ensured that the current usage is lookup free, but what's stopping someone from seeing that an API expects an InetSocketAddress and building one with the wrong constructor, adding a forward lookup? Or creating an InetSocketAddress around an InetAddress, then calling getHostName() call by mistake, adding a reverse lookup?
This is what made HostAndPort nice: it guarantees never doing any lookups. If you think switching away from HostAndPort is a big improvement, is there any way we can retain that guarantee? Maybe:
1. Extend InetSocketAddress and bake in our own enforcement, including throwing on getHostName(), and disallowing certain constructors.
2. Roll our own basic HostAndPort.
http://gerrit.cloudera.org:8080/#/c/11474/1/java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
File java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java:
http://gerrit.cloudera.org:8080/#/c/11474/1/java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java@178
PS1, Line 178: throw new IOException(resp.getError().getMessage());
> This was a carry-over from my WIP "breakout MiniKuduCluster to it's own Mod
I guess I don't understand the dependency issue; are you saying MiniKuduCluster can't depend on anything from kudu-client? What about the various protobufs that it uses? Or SecurityUtil?
I don't actually care about the exception.
http://gerrit.cloudera.org:8080/#/c/11474/1/java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java@297
PS1, Line 297: public void startMasterServer(InetSocketAddress address) throws IOException {
> Looking again, the MiniKuduCluster starts all the servers when `build()` is
I don't think you'll be able to get starting a server from scratch working; the C++ mini cluster doesn't support that.
Anyway, I don't care strongly about the naming; just wanted to make sure you were aware of the underlying limitation.
--
To view, visit http://gerrit.cloudera.org:8080/11474
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2a55c41f48f31c4a12b41508dbb343c4419e65b1
Gerrit-Change-Number: 11474
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Wed, 19 Sep 2018 21:12:47 +0000
Gerrit-HasComments: Yes
[kudu-CR] [test] Clean up MiniKuduCluster and BaseKuduTest
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11474 )
Change subject: [test] Clean up MiniKuduCluster and BaseKuduTest
......................................................................
Patch Set 4:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/11474/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11474/4//COMMIT_MSG@16
PS4, Line 16: it with our own HostAndPort which wraps an
Nit: extra leading space here.
http://gerrit.cloudera.org:8080/#/c/11474/4/java/kudu-client/src/main/java/org/apache/kudu/client/HostAndPort.java
File java/kudu-client/src/main/java/org/apache/kudu/client/HostAndPort.java:
PS4:
License header.
http://gerrit.cloudera.org:8080/#/c/11474/4/java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
File java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java:
http://gerrit.cloudera.org:8080/#/c/11474/4/java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java@295
PS4, Line 295: hostAndPort
Style nit: for local variables with really short scopes, I prefer shorter names. For example 'hp' here would be sufficient.
http://gerrit.cloudera.org:8080/#/c/11474/4/java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java@484
PS4, Line 484: address
Nit: the other methods earlier name this argument hostAndPort rather than address. Below too.
http://gerrit.cloudera.org:8080/#/c/11474/4/java/kudu-client/src/test/java/org/apache/kudu/util/TestNetUtil.java
File java/kudu-client/src/test/java/org/apache/kudu/util/TestNetUtil.java:
http://gerrit.cloudera.org:8080/#/c/11474/4/java/kudu-client/src/test/java/org/apache/kudu/util/TestNetUtil.java@38
PS4, Line 38: InetSocketAddress
HostAndPort
Below too.
--
To view, visit http://gerrit.cloudera.org:8080/11474
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2a55c41f48f31c4a12b41508dbb343c4419e65b1
Gerrit-Change-Number: 11474
Gerrit-PatchSet: 4
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Thu, 20 Sep 2018 22:19:40 +0000
Gerrit-HasComments: Yes
[kudu-CR] [test] Clean up MiniKuduCluster and BaseKuduTest
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/11474 )
Change subject: [test] Clean up MiniKuduCluster and BaseKuduTest
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11474/1/java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
File java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java:
http://gerrit.cloudera.org:8080/#/c/11474/1/java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java@297
PS1, Line 297: public void startMasterServer(InetSocketAddress address) throws IOException {
> Makes sense. I don't think it's possible (currently) to have a server that
Looking again, the MiniKuduCluster starts all the servers when `build()` is called. I don't think there is an issue.
--
To view, visit http://gerrit.cloudera.org:8080/11474
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2a55c41f48f31c4a12b41508dbb343c4419e65b1
Gerrit-Change-Number: 11474
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Wed, 19 Sep 2018 20:54:59 +0000
Gerrit-HasComments: Yes
[kudu-CR] [test] Clean up MiniKuduCluster and BaseKuduTest
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11474
to look at the new patch set (#3).
Change subject: [test] Clean up MiniKuduCluster and BaseKuduTest
......................................................................
[test] Clean up MiniKuduCluster and BaseKuduTest
This patch cleans up the MiniKuduCluster and
BaseKuduTest API in preparation for creating a
public test API and improving per-test
MiniKuduCluster parameters.
This patch also removes the use of HostAndPort as
part of the clean up. It replaces it with unresolved
InetSocketAddress because HostAndPort is provided
by Guava which is shaded and relocated in all of our
libraries. This means it shouldn’t be a part of the
“public” API. In this case that means the test API
too. Additionally HostAndPort is marked as beta.
Change-Id: I2a55c41f48f31c4a12b41508dbb343c4419e65b1
---
M java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala
M java/kudu-client-tools/src/test/java/org/apache/kudu/mapreduce/tools/ITExportCsv.java
M java/kudu-client-tools/src/test/java/org/apache/kudu/mapreduce/tools/ITImportCsv.java
M java/kudu-client-tools/src/test/java/org/apache/kudu/mapreduce/tools/ITImportParquet.java
M java/kudu-client-tools/src/test/java/org/apache/kudu/mapreduce/tools/ITImportParquetPreCheck.java
M java/kudu-client-tools/src/test/java/org/apache/kudu/mapreduce/tools/ITIntegrationTestBigLinkedList.java
M java/kudu-client-tools/src/test/java/org/apache/kudu/mapreduce/tools/ITRowCounter.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToCluster.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToClusterResponse.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java
M java/kudu-client/src/main/java/org/apache/kudu/util/NetUtil.java
M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java
M java/kudu-client/src/test/java/org/apache/kudu/client/ITClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/ITClientStress.java
M java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduSession.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestClientFailoverSupport.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectToCluster.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectionCache.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestHandleTooBusy.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestMasterFailover.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestMiniKuduCluster.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestMultipleLeaderFailover.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestNegotiation.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestRemoteTablet.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurity.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurityContextRealUser.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestServerInfo.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestTimeouts.java
M java/kudu-client/src/test/java/org/apache/kudu/util/TestNetUtil.java
M java/kudu-flume-sink/src/test/java/org/apache/kudu/flume/sink/AvroKuduOperationsProducerTest.java
M java/kudu-flume-sink/src/test/java/org/apache/kudu/flume/sink/KeyedKuduOperationsProducerTest.java
M java/kudu-flume-sink/src/test/java/org/apache/kudu/flume/sink/KuduSinkTest.java
M java/kudu-flume-sink/src/test/java/org/apache/kudu/flume/sink/RegexpKuduOperationsProducerTest.java
M java/kudu-mapreduce/src/test/java/org/apache/kudu/mapreduce/ITInputFormatJob.java
M java/kudu-mapreduce/src/test/java/org/apache/kudu/mapreduce/ITKuduTableInputFormat.java
M java/kudu-mapreduce/src/test/java/org/apache/kudu/mapreduce/ITKuduTableOutputFormat.java
M java/kudu-mapreduce/src/test/java/org/apache/kudu/mapreduce/ITOutputFormatJob.java
M java/kudu-spark-tools/src/test/scala/org/apache/kudu/spark/tools/ITBigLinkedListTest.scala
M java/kudu-spark-tools/src/test/scala/org/apache/kudu/spark/tools/TestImportExportFiles.scala
M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/DefaultSourceTest.scala
M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduContextTest.scala
M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduTestSuite.scala
47 files changed, 445 insertions(+), 382 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/11474/3
--
To view, visit http://gerrit.cloudera.org:8080/11474
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2a55c41f48f31c4a12b41508dbb343c4419e65b1
Gerrit-Change-Number: 11474
Gerrit-PatchSet: 3
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] [test] Clean up MiniKuduCluster and BaseKuduTest
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/11474 )
Change subject: [test] Clean up MiniKuduCluster and BaseKuduTest
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/11474/1/java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
File java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java:
http://gerrit.cloudera.org:8080/#/c/11474/1/java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java@178
PS1, Line 178: throw new RuntimeException(resp.getError().getMessage());
> Yeah, that is what I am saying. It will take some more changes to make that
I will undo the change in this patch and then make the change in the patch that needs it at a later point.
http://gerrit.cloudera.org:8080/#/c/11474/1/java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java@479
PS1, Line 479: /**
: * Converts HostPortPB to an unresolved InetSocketAddress.
: *
: * @param pb the host and port to convert
: * @return an unresolved InetSocketAddress
: */
: private static InetSocketAddress addressFromPB(HostPortPB pb) {
: return InetSocketAddress.createUnresolved(pb.getHost(), pb.getPort());
: }
> This isn't KUDU-2186. In fact that issue is fixed by removing HostAndPort f
I will reuse ProtobufHelper in this patch and then make the change in the patch that needs it at a later point.
--
To view, visit http://gerrit.cloudera.org:8080/11474
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2a55c41f48f31c4a12b41508dbb343c4419e65b1
Gerrit-Change-Number: 11474
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Thu, 20 Sep 2018 04:04:19 +0000
Gerrit-HasComments: Yes
[kudu-CR] [test] Clean up MiniKuduCluster and BaseKuduTest
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11474 )
Change subject: [test] Clean up MiniKuduCluster and BaseKuduTest
......................................................................
Patch Set 1:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/11474/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11474/1//COMMIT_MSG@11
PS1, Line 11: public test API
We're not committing to this with the same level of rigor or compatibility that we would our normal client API, right? What sort of guarantees do you have in mind? Could you spell them out?
http://gerrit.cloudera.org:8080/#/c/11474/1//COMMIT_MSG@15
PS1, Line 15: part of the clean up. It replaces it with unresolved InetSocketAddress because HostAndPort is provided
Nit: got a too long line here
http://gerrit.cloudera.org:8080/#/c/11474/1/java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
File java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java:
http://gerrit.cloudera.org:8080/#/c/11474/1/java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java@345
PS1, Line 345: public static InetSocketAddress AddressFromPB(Common.HostPortPB hostPortPB) {
Should be addressFromPB.
http://gerrit.cloudera.org:8080/#/c/11474/1/java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java
File java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java:
http://gerrit.cloudera.org:8080/#/c/11474/1/java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java@77
PS1, Line 77: getHostName()
Should we use getHostString() in place of getHostName()? The documentation says getHostName() may do a reverse lookup. Whether that happens or doesn't depends on how the InetSocketAddress was built (the lookup happens "if the address was created with a literal IP address"), but I don't know if that condition means the InetSocketAddress must have been constructed with a InetAddress argument, or with a hostname of "1.2.3.4" (see my other question about this in NetUtil).
http://gerrit.cloudera.org:8080/#/c/11474/1/java/kudu-client/src/main/java/org/apache/kudu/util/NetUtil.java
File java/kudu-client/src/main/java/org/apache/kudu/util/NetUtil.java:
http://gerrit.cloudera.org:8080/#/c/11474/1/java/kudu-client/src/main/java/org/apache/kudu/util/NetUtil.java@67
PS1, Line 67: return InetSocketAddress.createUnresolved(hostAndPort.getHost(), hostAndPort.getPort());
How does the resulting InetSocketAddress behave when the 'host' argument is actually an IP address string literal, like "1.2.3.4"? Does it still consider that string to be a hostname and the object is "unresolved"?
http://gerrit.cloudera.org:8080/#/c/11474/1/java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
File java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java:
http://gerrit.cloudera.org:8080/#/c/11474/1/java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java@178
PS1, Line 178: throw new RuntimeException(resp.getError().getMessage());
Why this change?
http://gerrit.cloudera.org:8080/#/c/11474/1/java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java@297
PS1, Line 297: public void startMasterServer(InetSocketAddress address) throws IOException {
FWIW, the reason these methods were prefixed with 'start' and not 'restart' is because you can't use them to start a server from scratch; you can only bring back up a server that was previously started when the cluster itself was started, and subsequently killed.
http://gerrit.cloudera.org:8080/#/c/11474/1/java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java@479
PS1, Line 479: /**
: * Converts HostPortPB to an unresolved InetSocketAddress.
: *
: * @param pb the host and port to convert
: * @return an unresolved InetSocketAddress
: */
: private static InetSocketAddress addressFromPB(HostPortPB pb) {
: return InetSocketAddress.createUnresolved(pb.getHost(), pb.getPort());
: }
Can't reuse the version in ProtobufHelper? Is this more of the TODO(KUDU-2186) thing from below?
--
To view, visit http://gerrit.cloudera.org:8080/11474
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2a55c41f48f31c4a12b41508dbb343c4419e65b1
Gerrit-Change-Number: 11474
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Wed, 19 Sep 2018 20:17:01 +0000
Gerrit-HasComments: Yes
[kudu-CR] [test] Clean up MiniKuduCluster and BaseKuduTest
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/11474 )
Change subject: [test] Clean up MiniKuduCluster and BaseKuduTest
......................................................................
Patch Set 4:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/11474/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11474/4//COMMIT_MSG@16
PS4, Line 16: it with our own HostAndPort which wraps an
> Nit: extra leading space here.
Done
http://gerrit.cloudera.org:8080/#/c/11474/4/java/kudu-client/src/main/java/org/apache/kudu/client/HostAndPort.java
File java/kudu-client/src/main/java/org/apache/kudu/client/HostAndPort.java:
PS4:
> License header.
Done
http://gerrit.cloudera.org:8080/#/c/11474/4/java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
File java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java:
http://gerrit.cloudera.org:8080/#/c/11474/4/java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java@295
PS4, Line 295: hostAndPort
> Style nit: for local variables with really short scopes, I prefer shorter n
Done
http://gerrit.cloudera.org:8080/#/c/11474/4/java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java@484
PS4, Line 484: address
> Nit: the other methods earlier name this argument hostAndPort rather than a
Done
http://gerrit.cloudera.org:8080/#/c/11474/4/java/kudu-client/src/test/java/org/apache/kudu/util/TestNetUtil.java
File java/kudu-client/src/test/java/org/apache/kudu/util/TestNetUtil.java:
http://gerrit.cloudera.org:8080/#/c/11474/4/java/kudu-client/src/test/java/org/apache/kudu/util/TestNetUtil.java@38
PS4, Line 38: InetSocketAddress
> HostAndPort
Done
--
To view, visit http://gerrit.cloudera.org:8080/11474
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2a55c41f48f31c4a12b41508dbb343c4419e65b1
Gerrit-Change-Number: 11474
Gerrit-PatchSet: 4
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Fri, 21 Sep 2018 00:42:20 +0000
Gerrit-HasComments: Yes
[kudu-CR] [test] Clean up MiniKuduCluster and BaseKuduTest
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11474
to look at the new patch set (#5).
Change subject: [test] Clean up MiniKuduCluster and BaseKuduTest
......................................................................
[test] Clean up MiniKuduCluster and BaseKuduTest
This patch cleans up the MiniKuduCluster and
BaseKuduTest API in preparation for creating a
public test API and improving per-test
MiniKuduCluster parameters.
This patch also removes the use of Guava’s
HostAndPort as part of the clean up. It replaces
it with our own HostAndPort which wraps an
unresolved InetSocketAddress. We can’t use
Guava’s HostAndPort because it is shaded and
relocated in all of our libraries. This means it shouldn’t
be a part of the “public” API. In this case that means
the test API too. Additionally HostAndPort is marked
as beta.
Change-Id: I2a55c41f48f31c4a12b41508dbb343c4419e65b1
---
M java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala
M java/kudu-client-tools/src/test/java/org/apache/kudu/mapreduce/tools/ITExportCsv.java
M java/kudu-client-tools/src/test/java/org/apache/kudu/mapreduce/tools/ITImportCsv.java
M java/kudu-client-tools/src/test/java/org/apache/kudu/mapreduce/tools/ITImportParquet.java
M java/kudu-client-tools/src/test/java/org/apache/kudu/mapreduce/tools/ITImportParquetPreCheck.java
M java/kudu-client-tools/src/test/java/org/apache/kudu/mapreduce/tools/ITIntegrationTestBigLinkedList.java
M java/kudu-client-tools/src/test/java/org/apache/kudu/mapreduce/tools/ITRowCounter.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToCluster.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToClusterResponse.java
A java/kudu-client/src/main/java/org/apache/kudu/client/HostAndPort.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java
M java/kudu-client/src/main/java/org/apache/kudu/util/NetUtil.java
M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java
M java/kudu-client/src/test/java/org/apache/kudu/client/ITClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/ITClientStress.java
M java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduSession.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestClientFailoverSupport.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectToCluster.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectionCache.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestHandleTooBusy.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestMasterFailover.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestMiniKuduCluster.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestMultipleLeaderFailover.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestNegotiation.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestRemoteTablet.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurity.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurityContextRealUser.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestServerInfo.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestTimeouts.java
M java/kudu-client/src/test/java/org/apache/kudu/util/TestNetUtil.java
M java/kudu-flume-sink/src/test/java/org/apache/kudu/flume/sink/AvroKuduOperationsProducerTest.java
M java/kudu-flume-sink/src/test/java/org/apache/kudu/flume/sink/KeyedKuduOperationsProducerTest.java
M java/kudu-flume-sink/src/test/java/org/apache/kudu/flume/sink/KuduSinkTest.java
M java/kudu-flume-sink/src/test/java/org/apache/kudu/flume/sink/RegexpKuduOperationsProducerTest.java
M java/kudu-mapreduce/src/test/java/org/apache/kudu/mapreduce/ITInputFormatJob.java
M java/kudu-mapreduce/src/test/java/org/apache/kudu/mapreduce/ITKuduTableInputFormat.java
M java/kudu-mapreduce/src/test/java/org/apache/kudu/mapreduce/ITKuduTableOutputFormat.java
M java/kudu-mapreduce/src/test/java/org/apache/kudu/mapreduce/ITOutputFormatJob.java
M java/kudu-spark-tools/src/test/scala/org/apache/kudu/spark/tools/ITBigLinkedListTest.scala
M java/kudu-spark-tools/src/test/scala/org/apache/kudu/spark/tools/TestImportExportFiles.scala
M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/DefaultSourceTest.scala
M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduContextTest.scala
M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduTestSuite.scala
48 files changed, 415 insertions(+), 292 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/11474/5
--
To view, visit http://gerrit.cloudera.org:8080/11474
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2a55c41f48f31c4a12b41508dbb343c4419e65b1
Gerrit-Change-Number: 11474
Gerrit-PatchSet: 5
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] [test] Clean up MiniKuduCluster and BaseKuduTest
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11474
to look at the new patch set (#4).
Change subject: [test] Clean up MiniKuduCluster and BaseKuduTest
......................................................................
[test] Clean up MiniKuduCluster and BaseKuduTest
This patch cleans up the MiniKuduCluster and
BaseKuduTest API in preparation for creating a
public test API and improving per-test
MiniKuduCluster parameters.
This patch also removes the use of Guava’s
HostAndPort as part of the clean up. It replaces
it with our own HostAndPort which wraps an
unresolved InetSocketAddress. We can’t use
Guava’s HostAndPort because it is shaded and
relocated in all of our libraries. This means it shouldn’t
be a part of the “public” API. In this case that means
the test API too. Additionally HostAndPort is marked
as beta.
Change-Id: I2a55c41f48f31c4a12b41508dbb343c4419e65b1
---
M java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala
M java/kudu-client-tools/src/test/java/org/apache/kudu/mapreduce/tools/ITExportCsv.java
M java/kudu-client-tools/src/test/java/org/apache/kudu/mapreduce/tools/ITImportCsv.java
M java/kudu-client-tools/src/test/java/org/apache/kudu/mapreduce/tools/ITImportParquet.java
M java/kudu-client-tools/src/test/java/org/apache/kudu/mapreduce/tools/ITImportParquetPreCheck.java
M java/kudu-client-tools/src/test/java/org/apache/kudu/mapreduce/tools/ITIntegrationTestBigLinkedList.java
M java/kudu-client-tools/src/test/java/org/apache/kudu/mapreduce/tools/ITRowCounter.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToCluster.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToClusterResponse.java
A java/kudu-client/src/main/java/org/apache/kudu/client/HostAndPort.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java
M java/kudu-client/src/main/java/org/apache/kudu/util/NetUtil.java
M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java
M java/kudu-client/src/test/java/org/apache/kudu/client/ITClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/ITClientStress.java
M java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduSession.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestClientFailoverSupport.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectToCluster.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectionCache.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestHandleTooBusy.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestMasterFailover.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestMiniKuduCluster.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestMultipleLeaderFailover.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestNegotiation.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestRemoteTablet.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurity.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurityContextRealUser.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestServerInfo.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestTimeouts.java
M java/kudu-client/src/test/java/org/apache/kudu/util/TestNetUtil.java
M java/kudu-flume-sink/src/test/java/org/apache/kudu/flume/sink/AvroKuduOperationsProducerTest.java
M java/kudu-flume-sink/src/test/java/org/apache/kudu/flume/sink/KeyedKuduOperationsProducerTest.java
M java/kudu-flume-sink/src/test/java/org/apache/kudu/flume/sink/KuduSinkTest.java
M java/kudu-flume-sink/src/test/java/org/apache/kudu/flume/sink/RegexpKuduOperationsProducerTest.java
M java/kudu-mapreduce/src/test/java/org/apache/kudu/mapreduce/ITInputFormatJob.java
M java/kudu-mapreduce/src/test/java/org/apache/kudu/mapreduce/ITKuduTableInputFormat.java
M java/kudu-mapreduce/src/test/java/org/apache/kudu/mapreduce/ITKuduTableOutputFormat.java
M java/kudu-mapreduce/src/test/java/org/apache/kudu/mapreduce/ITOutputFormatJob.java
M java/kudu-spark-tools/src/test/scala/org/apache/kudu/spark/tools/ITBigLinkedListTest.scala
M java/kudu-spark-tools/src/test/scala/org/apache/kudu/spark/tools/TestImportExportFiles.scala
M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/DefaultSourceTest.scala
M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduContextTest.scala
M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduTestSuite.scala
48 files changed, 408 insertions(+), 300 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/11474/4
--
To view, visit http://gerrit.cloudera.org:8080/11474
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2a55c41f48f31c4a12b41508dbb343c4419e65b1
Gerrit-Change-Number: 11474
Gerrit-PatchSet: 4
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] [test] Clean up MiniKuduCluster and BaseKuduTest
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11474 )
Change subject: [test] Clean up MiniKuduCluster and BaseKuduTest
......................................................................
[test] Clean up MiniKuduCluster and BaseKuduTest
This patch cleans up the MiniKuduCluster and
BaseKuduTest API in preparation for creating a
public test API and improving per-test
MiniKuduCluster parameters.
This patch also removes the use of Guava’s
HostAndPort as part of the clean up. It replaces
it with our own HostAndPort which wraps an
unresolved InetSocketAddress. We can’t use
Guava’s HostAndPort because it is shaded and
relocated in all of our libraries. This means it shouldn’t
be a part of the “public” API. In this case that means
the test API too. Additionally HostAndPort is marked
as beta.
Change-Id: I2a55c41f48f31c4a12b41508dbb343c4419e65b1
Reviewed-on: http://gerrit.cloudera.org:8080/11474
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
M java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala
M java/kudu-client-tools/src/test/java/org/apache/kudu/mapreduce/tools/ITExportCsv.java
M java/kudu-client-tools/src/test/java/org/apache/kudu/mapreduce/tools/ITImportCsv.java
M java/kudu-client-tools/src/test/java/org/apache/kudu/mapreduce/tools/ITImportParquet.java
M java/kudu-client-tools/src/test/java/org/apache/kudu/mapreduce/tools/ITImportParquetPreCheck.java
M java/kudu-client-tools/src/test/java/org/apache/kudu/mapreduce/tools/ITIntegrationTestBigLinkedList.java
M java/kudu-client-tools/src/test/java/org/apache/kudu/mapreduce/tools/ITRowCounter.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToCluster.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToClusterResponse.java
A java/kudu-client/src/main/java/org/apache/kudu/client/HostAndPort.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java
M java/kudu-client/src/main/java/org/apache/kudu/util/NetUtil.java
M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java
M java/kudu-client/src/test/java/org/apache/kudu/client/ITClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/ITClientStress.java
M java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduSession.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestClientFailoverSupport.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectToCluster.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectionCache.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestHandleTooBusy.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestMasterFailover.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestMiniKuduCluster.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestMultipleLeaderFailover.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestNegotiation.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestRemoteTablet.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurity.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurityContextRealUser.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestServerInfo.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestTimeouts.java
M java/kudu-client/src/test/java/org/apache/kudu/util/TestNetUtil.java
M java/kudu-flume-sink/src/test/java/org/apache/kudu/flume/sink/AvroKuduOperationsProducerTest.java
M java/kudu-flume-sink/src/test/java/org/apache/kudu/flume/sink/KeyedKuduOperationsProducerTest.java
M java/kudu-flume-sink/src/test/java/org/apache/kudu/flume/sink/KuduSinkTest.java
M java/kudu-flume-sink/src/test/java/org/apache/kudu/flume/sink/RegexpKuduOperationsProducerTest.java
M java/kudu-mapreduce/src/test/java/org/apache/kudu/mapreduce/ITInputFormatJob.java
M java/kudu-mapreduce/src/test/java/org/apache/kudu/mapreduce/ITKuduTableInputFormat.java
M java/kudu-mapreduce/src/test/java/org/apache/kudu/mapreduce/ITKuduTableOutputFormat.java
M java/kudu-mapreduce/src/test/java/org/apache/kudu/mapreduce/ITOutputFormatJob.java
M java/kudu-spark-tools/src/test/scala/org/apache/kudu/spark/tools/ITBigLinkedListTest.scala
M java/kudu-spark-tools/src/test/scala/org/apache/kudu/spark/tools/TestImportExportFiles.scala
M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/DefaultSourceTest.scala
M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduContextTest.scala
M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduTestSuite.scala
48 files changed, 415 insertions(+), 292 deletions(-)
Approvals:
Kudu Jenkins: Verified
Adar Dembo: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/11474
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I2a55c41f48f31c4a12b41508dbb343c4419e65b1
Gerrit-Change-Number: 11474
Gerrit-PatchSet: 6
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>