You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Anonymous Coward (Code Review)" <ge...@cloudera.org> on 2020/04/02 22:05:47 UTC
[kudu-CR] KUDU-3099: Remove System.exit() calls from KuduBackup/KuduRestore
waleed.fateem@gmail.com has uploaded this change for review. ( http://gerrit.cloudera.org:8080/15638
Change subject: KUDU-3099: Remove System.exit() calls from KuduBackup/KuduRestore
......................................................................
KUDU-3099: Remove System.exit() calls from KuduBackup/KuduRestore
The System.exit() calls have a side effect that can cause Spark to fail even
if the run function returns 0 on success. Rather than call System.exit()
the run() method will return true on a successful run. We then throw
a RuntimeException() in main if we find that run() failed, otherwise
we call SparkSession's stop() method to cleanly shutdown Spark.
Unfortunately the issue isn't easy to reproduce but we had one
environment exhibiting the problem and we confirmed that this patch
fixes the issue.
Change-Id: I7d1b4796b6280adecd7dab685a0281af6b2570ce
---
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala
2 files changed, 16 insertions(+), 12 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/38/15638/1
--
To view, visit http://gerrit.cloudera.org:8080/15638
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I7d1b4796b6280adecd7dab685a0281af6b2570ce
Gerrit-Change-Number: 15638
Gerrit-PatchSet: 1
Gerrit-Owner: Anonymous Coward <wa...@gmail.com>
[kudu-CR] KUDU-3099: Remove System.exit() calls from KuduBackup/KuduRestore
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/15638 )
Change subject: KUDU-3099: Remove System.exit() calls from KuduBackup/KuduRestore
......................................................................
Patch Set 3: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/15638
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7d1b4796b6280adecd7dab685a0281af6b2570ce
Gerrit-Change-Number: 15638
Gerrit-PatchSet: 3
Gerrit-Owner: Anonymous Coward <wa...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 06 Apr 2020 19:05:35 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-3099: Remove System.exit() calls from KuduBackup/KuduRestore
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/15638 )
Change subject: KUDU-3099: Remove System.exit() calls from KuduBackup/KuduRestore
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15638/1/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala
File java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala:
http://gerrit.cloudera.org:8080/#/c/15638/1/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala@174
PS1, Line 174: thrown
I think this is a typo and fails to compile. You can edit and test by using the below in the java directory:
`./gradlew :kudu-backup:check`
I also suspect you will need to adjust the tests slightly. Like here for example: https://github.com/apache/kudu/blob/master/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala#L698
--
To view, visit http://gerrit.cloudera.org:8080/15638
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7d1b4796b6280adecd7dab685a0281af6b2570ce
Gerrit-Change-Number: 15638
Gerrit-PatchSet: 1
Gerrit-Owner: Anonymous Coward <wa...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 03 Apr 2020 20:51:14 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3099: Remove System.exit() calls from KuduBackup/KuduRestore
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/15638 )
Change subject: KUDU-3099: Remove System.exit() calls from KuduBackup/KuduRestore
......................................................................
KUDU-3099: Remove System.exit() calls from KuduBackup/KuduRestore
The System.exit() calls have a side effect that can cause Spark to fail even
if the run function returns 0 on success. Rather than call System.exit()
the run() method will return true on a successful run. We then throw
a RuntimeException() in main if we find that run() failed, otherwise
we call SparkSession's stop() method to cleanly shutdown Spark.
Unfortunately the issue isn't easy to reproduce but we had one
environment exhibiting the problem and we confirmed that this patch
fixes the issue. TestKuduBackup.scala was modified where assertFalse()
is used to check for failure and assertTrue() for success.
Change-Id: I7d1b4796b6280adecd7dab685a0281af6b2570ce
Reviewed-on: http://gerrit.cloudera.org:8080/15638
Tested-by: Grant Henke <gr...@apache.org>
Reviewed-by: Grant Henke <gr...@apache.org>
---
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala
M java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala
3 files changed, 37 insertions(+), 33 deletions(-)
Approvals:
Grant Henke: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/15638
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I7d1b4796b6280adecd7dab685a0281af6b2570ce
Gerrit-Change-Number: 15638
Gerrit-PatchSet: 4
Gerrit-Owner: Anonymous Coward <wa...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-3099: Remove System.exit() calls from KuduBackup/KuduRestore
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Grant Henke,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15638
to look at the new patch set (#3).
Change subject: KUDU-3099: Remove System.exit() calls from KuduBackup/KuduRestore
......................................................................
KUDU-3099: Remove System.exit() calls from KuduBackup/KuduRestore
The System.exit() calls have a side effect that can cause Spark to fail even
if the run function returns 0 on success. Rather than call System.exit()
the run() method will return true on a successful run. We then throw
a RuntimeException() in main if we find that run() failed, otherwise
we call SparkSession's stop() method to cleanly shutdown Spark.
Unfortunately the issue isn't easy to reproduce but we had one
environment exhibiting the problem and we confirmed that this patch
fixes the issue. TestKuduBackup.scala was modified where assertFalse()
is used to check for failure and assertTrue() for success.
Change-Id: I7d1b4796b6280adecd7dab685a0281af6b2570ce
---
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala
M java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala
3 files changed, 37 insertions(+), 33 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/38/15638/3
--
To view, visit http://gerrit.cloudera.org:8080/15638
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7d1b4796b6280adecd7dab685a0281af6b2570ce
Gerrit-Change-Number: 15638
Gerrit-PatchSet: 3
Gerrit-Owner: Anonymous Coward <wa...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-3099: Remove System.exit() calls from KuduBackup/KuduRestore
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Grant Henke,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15638
to look at the new patch set (#2).
Change subject: KUDU-3099: Remove System.exit() calls from KuduBackup/KuduRestore
......................................................................
KUDU-3099: Remove System.exit() calls from KuduBackup/KuduRestore
The System.exit() calls have a side effect that can cause Spark to fail even
if the run function returns 0 on success. Rather than call System.exit()
the run() method will return true on a successful run. We then throw
a RuntimeException() in main if we find that run() failed, otherwise
we call SparkSession's stop() method to cleanly shutdown Spark.
Unfortunately the issue isn't easy to reproduce but we had one
environment exhibiting the problem and we confirmed that this patch
fixes the issue. TestKuduBackup.scala was modified where assertFalse()
is used to check for failure and assertTrue() for success.
Change-Id: I7d1b4796b6280adecd7dab685a0281af6b2570ce
---
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala
M java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala
3 files changed, 36 insertions(+), 32 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/38/15638/2
--
To view, visit http://gerrit.cloudera.org:8080/15638
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7d1b4796b6280adecd7dab685a0281af6b2570ce
Gerrit-Change-Number: 15638
Gerrit-PatchSet: 2
Gerrit-Owner: Anonymous Coward <wa...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-3099: Remove System.exit() calls from KuduBackup/KuduRestore
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has removed a vote on this change.
Change subject: KUDU-3099: Remove System.exit() calls from KuduBackup/KuduRestore
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/15638
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I7d1b4796b6280adecd7dab685a0281af6b2570ce
Gerrit-Change-Number: 15638
Gerrit-PatchSet: 3
Gerrit-Owner: Anonymous Coward <wa...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-3099: Remove System.exit() calls from KuduBackup/KuduRestore
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/15638 )
Change subject: KUDU-3099: Remove System.exit() calls from KuduBackup/KuduRestore
......................................................................
Patch Set 3: Code-Review+2
Thanks for the contribution!
--
To view, visit http://gerrit.cloudera.org:8080/15638
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7d1b4796b6280adecd7dab685a0281af6b2570ce
Gerrit-Change-Number: 15638
Gerrit-PatchSet: 3
Gerrit-Owner: Anonymous Coward <wa...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 06 Apr 2020 19:06:18 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-3099: Remove System.exit() calls from KuduBackup/KuduRestore
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/15638 )
Change subject: KUDU-3099: Remove System.exit() calls from KuduBackup/KuduRestore
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15638/2/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala
File java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala:
http://gerrit.cloudera.org:8080/#/c/15638/2/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala@175
PS2, Line 175: assertEquals(1, KuduBackup.run(options, ss)))
Missed this one.
--
To view, visit http://gerrit.cloudera.org:8080/15638
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7d1b4796b6280adecd7dab685a0281af6b2570ce
Gerrit-Change-Number: 15638
Gerrit-PatchSet: 2
Gerrit-Owner: Anonymous Coward <wa...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Sat, 04 Apr 2020 16:48:31 +0000
Gerrit-HasComments: Yes