You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@gobblin.apache.org by GitBox <gi...@apache.org> on 2020/09/10 21:14:56 UTC
[GitHub] [incubator-gobblin] arjun4084346 opened a new pull request #3100: [GOBBLIN-1260] add some logs in GobblinHelixTask
arjun4084346 opened a new pull request #3100:
URL: https://github.com/apache/incubator-gobblin/pull/3100
Dear Gobblin maintainers,
Please accept this PR. I understand that it will not be reviewed until I have checked off all the steps below!
### JIRA
- [x] My PR addresses the following [Gobblin JIRA](https://issues.apache.org/jira/browse/GOBBLIN/) issues and references them in the PR title. For example, "[GOBBLIN-XXX] My Gobblin PR"
- https://issues.apache.org/jira/browse/GOBBLIN-1260
### Description
- [x] Here are some details about my PR, including screenshots (if applicable):
add some logs to debug if helix task cancel is throwing exceptions or hanging
### Tests
- [x] My PR adds the following unit tests __OR__ does not need testing for this extremely good reason:
trivial changes
### Commits
- [x] My commits all reference JIRA issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
2. Subject is limited to 50 characters
3. Subject does not end with a period
4. Subject uses the imperative mood ("add", not "adding")
5. Body wraps at 72 characters
6. Body explains "what" and "why", not "how"
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-gobblin] sv2000 commented on a change in pull request #3100: [GOBBLIN-1260] add some logs in GobblinHelixTask
Posted by GitBox <gi...@apache.org>.
sv2000 commented on a change in pull request #3100:
URL: https://github.com/apache/incubator-gobblin/pull/3100#discussion_r487138511
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
Review comment:
As @autumnust has pointed out to me, the pattern of catching an exception to log and re-throw the same exception is an anti-pattern. I would recommend reading this SO post: https://softwareengineering.stackexchange.com/questions/365427/try-catch-log-rethrow-is-anti-pattern
In your case, I think you are catching the exception to log the jobId. In this case, per the SO post, it is recommended to wrap the original exception into another exception with the additional context and propagate it up the call stack.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-gobblin] arjun4084346 commented on a change in pull request #3100: [GOBBLIN-1260] add some logs in GobblinHelixTask
Posted by GitBox <gi...@apache.org>.
arjun4084346 commented on a change in pull request #3100:
URL: https://github.com/apache/incubator-gobblin/pull/3100#discussion_r487198150
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
Review comment:
I do not intend to just catch and throw the Throwable. I wanted to log the exception in gobblin's log. This method is called from helix class and I do not want to depend on helix code on exception handling.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-gobblin] sv2000 commented on a change in pull request #3100: [GOBBLIN-1260] add some logs in GobblinHelixTask
Posted by GitBox <gi...@apache.org>.
sv2000 commented on a change in pull request #3100:
URL: https://github.com/apache/incubator-gobblin/pull/3100#discussion_r487138511
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
Review comment:
As @autumnust has pointed out to me, the pattern of catching an exception to log and re-throw the same exception is an anti-pattern. I would recommend reading this SO post: https://softwareengineering.stackexchange.com/questions/365427/try-catch-log-rethrow-is-anti-pattern
In your case, I think you are catching the exception to log the jobId. In this case, per the SO post, it is recommended to wrap the original exception into another exception with the additional context and propagate it up the call stack.
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
+ throw new RuntimeException("Gobblin helix task cancellation for jobId " + jobId + " failed with exception.", t);
Review comment:
Why not wrap it into a Throwable instead of a RuntimeException?
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
Review comment:
I don't think this log line is needed, given that the RuntimeException already has the context. This will pollute the logs with duplicate stacktraces.
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
+ throw new RuntimeException("Gobblin helix task cancellation for jobId " + jobId + " failed with exception.", t);
Review comment:
Ah I see. Makes sense.
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
+ throw new RuntimeException("Gobblin helix task cancellation for jobId " + jobId + " failed with exception.", t);
Review comment:
Can we just do Throwables.propagate(t), which essentially wraps the throwable into a RuntimeException?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-gobblin] arjun4084346 commented on a change in pull request #3100: [GOBBLIN-1260] add some logs in GobblinHelixTask
Posted by GitBox <gi...@apache.org>.
arjun4084346 commented on a change in pull request #3100:
URL: https://github.com/apache/incubator-gobblin/pull/3100#discussion_r487198150
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
Review comment:
I do not intend to just catch and throw the Throwable. I wanted to log the exception in gobblin's log. This method is called from helix class and I do not want to depend on helix code on exception handling.
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
+ throw new RuntimeException("Gobblin helix task cancellation for jobId " + jobId + " failed with exception.", t);
Review comment:
the method signature does not throw any exception. RuntimeException is Unchecked exception and can be thrown without changing the signature. Throwable is not Unchecked exception.
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
Review comment:
I do not intend to just catch and throw the Throwable. I wanted to log the exception in gobblin's log. This method is called from helix class and I do not want to depend on helix code on exception handling.
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
+ throw new RuntimeException("Gobblin helix task cancellation for jobId " + jobId + " failed with exception.", t);
Review comment:
the method signature does not throw any exception. RuntimeException is Unchecked exception and can be thrown without changing the signature. Throwable is not Unchecked exception.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-gobblin] sv2000 commented on a change in pull request #3100: [GOBBLIN-1260] add some logs in GobblinHelixTask
Posted by GitBox <gi...@apache.org>.
sv2000 commented on a change in pull request #3100:
URL: https://github.com/apache/incubator-gobblin/pull/3100#discussion_r487226253
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
+ throw new RuntimeException("Gobblin helix task cancellation for jobId " + jobId + " failed with exception.", t);
Review comment:
Ah I see. Makes sense.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-gobblin] sv2000 commented on a change in pull request #3100: [GOBBLIN-1260] add some logs in GobblinHelixTask
Posted by GitBox <gi...@apache.org>.
sv2000 commented on a change in pull request #3100:
URL: https://github.com/apache/incubator-gobblin/pull/3100#discussion_r487138511
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
Review comment:
As @autumnust has pointed out to me, the pattern of catching an exception to log and re-throw the same exception is an anti-pattern. I would recommend reading this SO post: https://softwareengineering.stackexchange.com/questions/365427/try-catch-log-rethrow-is-anti-pattern
In your case, I think you are catching the exception to log the jobId. In this case, per the SO post, it is recommended to wrap the original exception into another exception with the additional context and propagate it up the call stack.
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
+ throw new RuntimeException("Gobblin helix task cancellation for jobId " + jobId + " failed with exception.", t);
Review comment:
Why not wrap it into a Throwable instead of a RuntimeException?
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
Review comment:
I don't think this log line is needed, given that the RuntimeException already has the context. This will pollute the logs with duplicate stacktraces.
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
+ throw new RuntimeException("Gobblin helix task cancellation for jobId " + jobId + " failed with exception.", t);
Review comment:
Ah I see. Makes sense.
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
+ throw new RuntimeException("Gobblin helix task cancellation for jobId " + jobId + " failed with exception.", t);
Review comment:
Can we just do Throwables.propagate(t), which essentially wraps the throwable into a RuntimeException?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-gobblin] sv2000 commented on a change in pull request #3100: [GOBBLIN-1260] add some logs in GobblinHelixTask
Posted by GitBox <gi...@apache.org>.
sv2000 commented on a change in pull request #3100:
URL: https://github.com/apache/incubator-gobblin/pull/3100#discussion_r487323899
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
+ throw new RuntimeException("Gobblin helix task cancellation for jobId " + jobId + " failed with exception.", t);
Review comment:
Can we just do Throwables.propagate(t), which essentially wraps the throwable into a RuntimeException?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-gobblin] sv2000 commented on a change in pull request #3100: [GOBBLIN-1260] add some logs in GobblinHelixTask
Posted by GitBox <gi...@apache.org>.
sv2000 commented on a change in pull request #3100:
URL: https://github.com/apache/incubator-gobblin/pull/3100#discussion_r487138511
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
Review comment:
As @autumnust has pointed out to me, the pattern of catching an exception to log and re-throw the same exception is an anti-pattern. I would recommend reading this SO post: https://softwareengineering.stackexchange.com/questions/365427/try-catch-log-rethrow-is-anti-pattern
In your case, I think you are catching the exception to log the jobId. In this case, per the SO post, it is recommended to wrap the original exception into another exception with the additional context and propagate it up the call stack.
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
+ throw new RuntimeException("Gobblin helix task cancellation for jobId " + jobId + " failed with exception.", t);
Review comment:
Why not wrap it into a Throwable instead of a RuntimeException?
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
Review comment:
I don't think this log line is needed, given that the RuntimeException already has the context. This will pollute the logs with duplicate stacktraces.
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
+ throw new RuntimeException("Gobblin helix task cancellation for jobId " + jobId + " failed with exception.", t);
Review comment:
Ah I see. Makes sense.
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
+ throw new RuntimeException("Gobblin helix task cancellation for jobId " + jobId + " failed with exception.", t);
Review comment:
Can we just do Throwables.propagate(t), which essentially wraps the throwable into a RuntimeException?
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
Review comment:
As @autumnust has pointed out to me, the pattern of catching an exception to log and re-throw the same exception is an anti-pattern. I would recommend reading this SO post: https://softwareengineering.stackexchange.com/questions/365427/try-catch-log-rethrow-is-anti-pattern
In your case, I think you are catching the exception to log the jobId. In this case, per the SO post, it is recommended to wrap the original exception into another exception with the additional context and propagate it up the call stack.
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
+ throw new RuntimeException("Gobblin helix task cancellation for jobId " + jobId + " failed with exception.", t);
Review comment:
Why not wrap it into a Throwable instead of a RuntimeException?
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
Review comment:
I don't think this log line is needed, given that the RuntimeException already has the context. This will pollute the logs with duplicate stacktraces.
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
+ throw new RuntimeException("Gobblin helix task cancellation for jobId " + jobId + " failed with exception.", t);
Review comment:
Ah I see. Makes sense.
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
+ throw new RuntimeException("Gobblin helix task cancellation for jobId " + jobId + " failed with exception.", t);
Review comment:
Can we just do Throwables.propagate(t), which essentially wraps the throwable into a RuntimeException?
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
Review comment:
As @autumnust has pointed out to me, the pattern of catching an exception to log and re-throw the same exception is an anti-pattern. I would recommend reading this SO post: https://softwareengineering.stackexchange.com/questions/365427/try-catch-log-rethrow-is-anti-pattern
In your case, I think you are catching the exception to log the jobId. In this case, per the SO post, it is recommended to wrap the original exception into another exception with the additional context and propagate it up the call stack.
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
+ throw new RuntimeException("Gobblin helix task cancellation for jobId " + jobId + " failed with exception.", t);
Review comment:
Why not wrap it into a Throwable instead of a RuntimeException?
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
Review comment:
I don't think this log line is needed, given that the RuntimeException already has the context. This will pollute the logs with duplicate stacktraces.
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
+ throw new RuntimeException("Gobblin helix task cancellation for jobId " + jobId + " failed with exception.", t);
Review comment:
Ah I see. Makes sense.
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
+ throw new RuntimeException("Gobblin helix task cancellation for jobId " + jobId + " failed with exception.", t);
Review comment:
Can we just do Throwables.propagate(t), which essentially wraps the throwable into a RuntimeException?
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
Review comment:
As @autumnust has pointed out to me, the pattern of catching an exception to log and re-throw the same exception is an anti-pattern. I would recommend reading this SO post: https://softwareengineering.stackexchange.com/questions/365427/try-catch-log-rethrow-is-anti-pattern
In your case, I think you are catching the exception to log the jobId. In this case, per the SO post, it is recommended to wrap the original exception into another exception with the additional context and propagate it up the call stack.
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
+ throw new RuntimeException("Gobblin helix task cancellation for jobId " + jobId + " failed with exception.", t);
Review comment:
Why not wrap it into a Throwable instead of a RuntimeException?
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
Review comment:
I don't think this log line is needed, given that the RuntimeException already has the context. This will pollute the logs with duplicate stacktraces.
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
+ throw new RuntimeException("Gobblin helix task cancellation for jobId " + jobId + " failed with exception.", t);
Review comment:
Ah I see. Makes sense.
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
+ throw new RuntimeException("Gobblin helix task cancellation for jobId " + jobId + " failed with exception.", t);
Review comment:
Can we just do Throwables.propagate(t), which essentially wraps the throwable into a RuntimeException?
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
Review comment:
As @autumnust has pointed out to me, the pattern of catching an exception to log and re-throw the same exception is an anti-pattern. I would recommend reading this SO post: https://softwareengineering.stackexchange.com/questions/365427/try-catch-log-rethrow-is-anti-pattern
In your case, I think you are catching the exception to log the jobId. In this case, per the SO post, it is recommended to wrap the original exception into another exception with the additional context and propagate it up the call stack.
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
+ throw new RuntimeException("Gobblin helix task cancellation for jobId " + jobId + " failed with exception.", t);
Review comment:
Why not wrap it into a Throwable instead of a RuntimeException?
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
Review comment:
I don't think this log line is needed, given that the RuntimeException already has the context. This will pollute the logs with duplicate stacktraces.
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
+ throw new RuntimeException("Gobblin helix task cancellation for jobId " + jobId + " failed with exception.", t);
Review comment:
Ah I see. Makes sense.
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
+ throw new RuntimeException("Gobblin helix task cancellation for jobId " + jobId + " failed with exception.", t);
Review comment:
Can we just do Throwables.propagate(t), which essentially wraps the throwable into a RuntimeException?
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
Review comment:
As @autumnust has pointed out to me, the pattern of catching an exception to log and re-throw the same exception is an anti-pattern. I would recommend reading this SO post: https://softwareengineering.stackexchange.com/questions/365427/try-catch-log-rethrow-is-anti-pattern
In your case, I think you are catching the exception to log the jobId. In this case, per the SO post, it is recommended to wrap the original exception into another exception with the additional context and propagate it up the call stack.
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
+ throw new RuntimeException("Gobblin helix task cancellation for jobId " + jobId + " failed with exception.", t);
Review comment:
Why not wrap it into a Throwable instead of a RuntimeException?
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
Review comment:
I don't think this log line is needed, given that the RuntimeException already has the context. This will pollute the logs with duplicate stacktraces.
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
+ throw new RuntimeException("Gobblin helix task cancellation for jobId " + jobId + " failed with exception.", t);
Review comment:
Ah I see. Makes sense.
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
+ throw new RuntimeException("Gobblin helix task cancellation for jobId " + jobId + " failed with exception.", t);
Review comment:
Can we just do Throwables.propagate(t), which essentially wraps the throwable into a RuntimeException?
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
Review comment:
As @autumnust has pointed out to me, the pattern of catching an exception to log and re-throw the same exception is an anti-pattern. I would recommend reading this SO post: https://softwareengineering.stackexchange.com/questions/365427/try-catch-log-rethrow-is-anti-pattern
In your case, I think you are catching the exception to log the jobId. In this case, per the SO post, it is recommended to wrap the original exception into another exception with the additional context and propagate it up the call stack.
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
+ throw new RuntimeException("Gobblin helix task cancellation for jobId " + jobId + " failed with exception.", t);
Review comment:
Why not wrap it into a Throwable instead of a RuntimeException?
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
Review comment:
I don't think this log line is needed, given that the RuntimeException already has the context. This will pollute the logs with duplicate stacktraces.
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
+ throw new RuntimeException("Gobblin helix task cancellation for jobId " + jobId + " failed with exception.", t);
Review comment:
Ah I see. Makes sense.
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
+ throw new RuntimeException("Gobblin helix task cancellation for jobId " + jobId + " failed with exception.", t);
Review comment:
Can we just do Throwables.propagate(t), which essentially wraps the throwable into a RuntimeException?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-gobblin] asfgit closed pull request #3100: [GOBBLIN-1260] add some logs in GobblinHelixTask
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #3100:
URL: https://github.com/apache/incubator-gobblin/pull/3100
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-gobblin] sv2000 commented on a change in pull request #3100: [GOBBLIN-1260] add some logs in GobblinHelixTask
Posted by GitBox <gi...@apache.org>.
sv2000 commented on a change in pull request #3100:
URL: https://github.com/apache/incubator-gobblin/pull/3100#discussion_r487191721
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
+ throw new RuntimeException("Gobblin helix task cancellation for jobId " + jobId + " failed with exception.", t);
Review comment:
Why not wrap it into a Throwable instead of a RuntimeException?
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
Review comment:
I don't think this log line is needed, given that the RuntimeException already has the context. This will pollute the logs with duplicate stacktraces.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-gobblin] arjun4084346 commented on a change in pull request #3100: [GOBBLIN-1260] add some logs in GobblinHelixTask
Posted by GitBox <gi...@apache.org>.
arjun4084346 commented on a change in pull request #3100:
URL: https://github.com/apache/incubator-gobblin/pull/3100#discussion_r487198150
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
Review comment:
I do not intend to just catch and throw the Throwable. I wanted to log the exception in gobblin's log. This method is called from helix class and I do not want to depend on helix code on exception handling.
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
+ throw new RuntimeException("Gobblin helix task cancellation for jobId " + jobId + " failed with exception.", t);
Review comment:
the method signature does not throw any exception. RuntimeException is Unchecked exception and can be thrown without changing the signature. Throwable is not Unchecked exception.
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
Review comment:
I do not intend to just catch and throw the Throwable. I wanted to log the exception in gobblin's log. This method is called from helix class and I do not want to depend on helix code on exception handling.
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
+ throw new RuntimeException("Gobblin helix task cancellation for jobId " + jobId + " failed with exception.", t);
Review comment:
the method signature does not throw any exception. RuntimeException is Unchecked exception and can be thrown without changing the signature. Throwable is not Unchecked exception.
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
Review comment:
I do not intend to just catch and throw the Throwable. I wanted to log the exception in gobblin's log. This method is called from helix class and I do not want to depend on helix code on exception handling.
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
+ throw new RuntimeException("Gobblin helix task cancellation for jobId " + jobId + " failed with exception.", t);
Review comment:
the method signature does not throw any exception. RuntimeException is Unchecked exception and can be thrown without changing the signature. Throwable is not Unchecked exception.
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
Review comment:
I do not intend to just catch and throw the Throwable. I wanted to log the exception in gobblin's log. This method is called from helix class and I do not want to depend on helix code on exception handling.
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
+ throw new RuntimeException("Gobblin helix task cancellation for jobId " + jobId + " failed with exception.", t);
Review comment:
the method signature does not throw any exception. RuntimeException is Unchecked exception and can be thrown without changing the signature. Throwable is not Unchecked exception.
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
Review comment:
I do not intend to just catch and throw the Throwable. I wanted to log the exception in gobblin's log. This method is called from helix class and I do not want to depend on helix code on exception handling.
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
+ throw new RuntimeException("Gobblin helix task cancellation for jobId " + jobId + " failed with exception.", t);
Review comment:
the method signature does not throw any exception. RuntimeException is Unchecked exception and can be thrown without changing the signature. Throwable is not Unchecked exception.
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
Review comment:
I do not intend to just catch and throw the Throwable. I wanted to log the exception in gobblin's log. This method is called from helix class and I do not want to depend on helix code on exception handling.
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
+ throw new RuntimeException("Gobblin helix task cancellation for jobId " + jobId + " failed with exception.", t);
Review comment:
the method signature does not throw any exception. RuntimeException is Unchecked exception and can be thrown without changing the signature. Throwable is not Unchecked exception.
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
Review comment:
I do not intend to just catch and throw the Throwable. I wanted to log the exception in gobblin's log. This method is called from helix class and I do not want to depend on helix code on exception handling.
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
+ throw new RuntimeException("Gobblin helix task cancellation for jobId " + jobId + " failed with exception.", t);
Review comment:
the method signature does not throw any exception. RuntimeException is Unchecked exception and can be thrown without changing the signature. Throwable is not Unchecked exception.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-gobblin] arjun4084346 commented on a change in pull request #3100: [GOBBLIN-1260] add some logs in GobblinHelixTask
Posted by GitBox <gi...@apache.org>.
arjun4084346 commented on a change in pull request #3100:
URL: https://github.com/apache/incubator-gobblin/pull/3100#discussion_r487199787
##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
##########
@@ -213,9 +213,17 @@ private Integer getPartitionForHelixTask(TaskDriver taskDriver) {
@Override
public void cancel() {
- log.info("Gobblin helix task cancellation invoked.");
+ log.info("Gobblin helix task cancellation invoked for jobId {}.", jobId);
if (this.task != null ) {
- this.task.cancel();
+ try {
+ this.task.cancel();
+ log.info("Gobblin helix task cancellation completed for jobId {}.", jobId);
+ } catch (Throwable t) {
+ log.info("Gobblin helix task cancellation for jobId {} failed with exception.", jobId, t);
+ throw new RuntimeException("Gobblin helix task cancellation for jobId " + jobId + " failed with exception.", t);
Review comment:
the method signature does not throw any exception. RuntimeException is Unchecked exception and can be thrown without changing the signature. Throwable is not Unchecked exception.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org