Skip to content

Conversation

ghost
Copy link

@ghost ghost commented Jun 26, 2017

No description provided.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 82.036% when pulling 64d9606 on liubao68:modify_log into c1d4e2c on ServiceComb:master.

return e; //by default, just pass through
}
});
} catch (Exception e) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only catch exception type thrown by the method called

Copy link
Contributor

@liubao68 liubao68 Jun 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no exception defined by this method, only RuntimeException will throw by this method. Here we ignore any runtimeexception thrown by this method and only give a warning.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it throws IllegalStateException and we shall only catch this one.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's a good practice. Because IllegalStateException is thrown internally, we don't know what's it if we don't check the source code. This rule is acceptable when the exception is declared by the method implicitly and not a RuntimeException. When we deal with RuntimeException, we'd better to use generic exception type, It's better for code quality.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you don't log the actual exception message, it could mislead the user if the RuntimeException is not related to HystrixCommandExcecution hook setting.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.05%) to 82.003% when pulling 43bd8c5 on liubao68:modify_log into c1d4e2c on ServiceComb:master.

@seanyinx seanyinx merged commit 9903784 into apache:master Jun 26, 2017
@liubao68 liubao68 deleted the modify_log branch June 26, 2017 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants