You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Adar Dembo (Code Review)" <ge...@cloudera.org> on 2016/06/06 21:45:51 UTC
[kudu-CR] [java client] fix reruns of TestKuduTable.testGetLocations
Hello Dan Burkert, Jean-Daniel Cryans,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/3319
to review the following change.
Change subject: [java client] fix reruns of TestKuduTable.testGetLocations
......................................................................
[java client] fix reruns of TestKuduTable.testGetLocations
The test expected a table count including the table created in
testAlterTable, which meant that surefire reruns would always fail, as they
clean up the minicluster state (via @AfterClass) but only run the particular
failed test.
Why not generalize? My thought process went something like this:
1. Let's add an @After to BaseKuduTest that enumerates all tables and
deletes them.
2. #1 is a tax on every test, and besides, if we're going to undo all
destructive changes, we should also restart stopped procseses.
3. If we're going to do #2, we may as well convert the existing @BeforeClass
and @AfterClass into @Before and @After instead, since that's the same
thing semantically but less code and guaranteed to capture every change.
4. But cluster setup in the Java tests is slow due to the Thread.sleep(300)
performed by every started daemon. Let's do what we do in C++ tests and
ask the servers to dump a file, sleeping until that file appears.
5. #4 is a lot of refactoring and doesn't address the multiple master case
well. Let's punt on the whole endeavour and do a targeted fix instead.
Change-Id: I44220d3c5ee1816f2bcf345c6062d33616145da2
---
M java/kudu-client/src/test/java/org/kududb/client/TestKuduTable.java
1 file changed, 30 insertions(+), 22 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/19/3319/1
--
To view, visit http://gerrit.cloudera.org:8080/3319
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I44220d3c5ee1816f2bcf345c6062d33616145da2
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans
[kudu-CR] [java client] fix reruns of TestKuduTable.testGetLocations
Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.
Jean-Daniel Cryans has posted comments on this change.
Change subject: [java client] fix reruns of TestKuduTable.testGetLocations
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/3319
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I44220d3c5ee1816f2bcf345c6062d33616145da2
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] [java client] fix reruns of TestKuduTable.testGetLocations
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: [java client] fix reruns of TestKuduTable.testGetLocations
......................................................................
Patch Set 2:
Build Started http://104.196.14.100/job/kudu-gerrit/1751/
--
To view, visit http://gerrit.cloudera.org:8080/3319
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I44220d3c5ee1816f2bcf345c6062d33616145da2
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] [java client] fix reruns of TestKuduTable.testGetLocations
Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.
Jean-Daniel Cryans has submitted this change and it was merged.
Change subject: [java client] fix reruns of TestKuduTable.testGetLocations
......................................................................
[java client] fix reruns of TestKuduTable.testGetLocations
The test expected a table count including the table created in
testAlterTable, which meant that surefire reruns would always fail, as they
clean up the minicluster state (via @AfterClass) but only run the particular
failed test.
Why not generalize? My thought process went something like this:
1. Let's add an @After to BaseKuduTest that enumerates all tables and
deletes them.
2. #1 is a tax on every test, and besides, if we're going to undo all
destructive changes, we should also restart stopped processes.
3. If we're going to do #2, we may as well convert the existing @BeforeClass
and @AfterClass into @Before and @After instead, since that's the same
thing semantically but less code and guaranteed to capture every change.
4. But cluster setup in the Java tests is slow due to the Thread.sleep(300)
performed by every started daemon. Let's do what we do in C++ tests and
ask the servers to dump a file, sleeping until that file appears.
5. #4 is a lot of refactoring and doesn't address the multiple master case
well. Let's punt on the whole endeavour and do a targeted fix instead.
Change-Id: I44220d3c5ee1816f2bcf345c6062d33616145da2
Reviewed-on: http://gerrit.cloudera.org:8080/3319
Tested-by: Kudu Jenkins
Reviewed-by: Jean-Daniel Cryans
---
M java/kudu-client/src/test/java/org/kududb/client/TestKuduTable.java
1 file changed, 30 insertions(+), 22 deletions(-)
Approvals:
Jean-Daniel Cryans: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/3319
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I44220d3c5ee1816f2bcf345c6062d33616145da2
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [java client] fix reruns of TestKuduTable.testGetLocations
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: [java client] fix reruns of TestKuduTable.testGetLocations
......................................................................
Patch Set 2:
(1 comment)
LGTM, just the small nit.
http://gerrit.cloudera.org:8080/#/c/3319/2//COMMIT_MSG
Commit Message:
PS2, Line 18: procseses
processes
--
To view, visit http://gerrit.cloudera.org:8080/3319
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I44220d3c5ee1816f2bcf345c6062d33616145da2
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] [java client] fix reruns of TestKuduTable.testGetLocations
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: [java client] fix reruns of TestKuduTable.testGetLocations
......................................................................
Patch Set 1:
Build Started http://104.196.14.100/job/kudu-gerrit/1750/
--
To view, visit http://gerrit.cloudera.org:8080/3319
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I44220d3c5ee1816f2bcf345c6062d33616145da2
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] [java client] fix reruns of TestKuduTable.testGetLocations
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: [java client] fix reruns of TestKuduTable.testGetLocations
......................................................................
Patch Set 3:
Build Started http://104.196.14.100/job/kudu-gerrit/1759/
--
To view, visit http://gerrit.cloudera.org:8080/3319
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I44220d3c5ee1816f2bcf345c6062d33616145da2
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] [java client] fix reruns of TestKuduTable.testGetLocations
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3319
to look at the new patch set (#3).
Change subject: [java client] fix reruns of TestKuduTable.testGetLocations
......................................................................
[java client] fix reruns of TestKuduTable.testGetLocations
The test expected a table count including the table created in
testAlterTable, which meant that surefire reruns would always fail, as they
clean up the minicluster state (via @AfterClass) but only run the particular
failed test.
Why not generalize? My thought process went something like this:
1. Let's add an @After to BaseKuduTest that enumerates all tables and
deletes them.
2. #1 is a tax on every test, and besides, if we're going to undo all
destructive changes, we should also restart stopped processes.
3. If we're going to do #2, we may as well convert the existing @BeforeClass
and @AfterClass into @Before and @After instead, since that's the same
thing semantically but less code and guaranteed to capture every change.
4. But cluster setup in the Java tests is slow due to the Thread.sleep(300)
performed by every started daemon. Let's do what we do in C++ tests and
ask the servers to dump a file, sleeping until that file appears.
5. #4 is a lot of refactoring and doesn't address the multiple master case
well. Let's punt on the whole endeavour and do a targeted fix instead.
Change-Id: I44220d3c5ee1816f2bcf345c6062d33616145da2
---
M java/kudu-client/src/test/java/org/kududb/client/TestKuduTable.java
1 file changed, 30 insertions(+), 22 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/19/3319/3
--
To view, visit http://gerrit.cloudera.org:8080/3319
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I44220d3c5ee1816f2bcf345c6062d33616145da2
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins