Skip to content

Conversation

@supratimdeka
Copy link
Contributor

https://issues.apache.org/jira/browse/HDDS-2208

This is a follow-up to the patch for HDDS-2206
#12

The change propagates complete stacktraces for system exceptions encountered during the Ratis phase of the OM request handling.

However, this patch does not consider the configuration parameter introduced earlier in the patch for HDDS-2206. Controlling the behaviour using a configuration parameter requires a much greater footprint in the code. Because at this point, there is no clear requirement for such a config parameter - going ahead without the config param.
Will update the patch for HDDS-2206 as well - will remove the configuration that was introduced.

Copy link
Contributor

@bharatviswa504 bharatviswa504 left a comment

Choose a reason for hiding this comment

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

Overall LGTM.
What is the end goal of this change or instead of just the exception message, have the entire stackTrace for System exceptions that is only the intention here?

@supratimdeka
Copy link
Contributor Author

@bharatviswa504 , yes the intention is to send the entire stack trace for system exceptions to the client. the expectation is to make debugging more convenient.

the original intent was to throw exceptions on the client (via protobuf ServiceExceptions). but that gets quite complicated because of Ratis. Lot simpler to make the same information available in the message field of the OM response. So leaving it at that for now.

@anuengineer
Copy link
Contributor

Can we have this disable by default via a config flag. This is a debug facility and should remain so.

@elek
Copy link
Member

elek commented Oct 31, 2019

Can we have this disable by default via a config flag. This is a debug facility and should remain so.

Depends what is debug facility ;-) As I understood this posts the stack trace only in case of an unexpected exception. All of the expected exceptions are handled by OMException-s where the stack trace is not included.

I think it's good to have the stack trace in these cases. Would be better to have it in a structured form, but as it's explained this is the easiest way.

Let me commit this, and we can continue the discussion if any configuration key is required (and if it is, we can add it in a follow-up jira)

Copy link
Member

@elek elek left a comment

Choose a reason for hiding this comment

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

+1

Thanks the contribution @supratimdeka

@elek elek closed this in b6704fd Oct 31, 2019
@elek
Copy link
Member

elek commented Nov 4, 2019

I discussed it with @anuengineer and understood that he has more concerns regarding the propagation of stack traces to the client, as it couldn't be used easily with any non-java client in the future.

I also found this comment: https://issues.apache.org/jira/browse/HDDS-2206

Reverted this based on offline conversation with Anu Engineer.

Anu has requested we add a config key to control this behavior.

But it's hard to understand the concerns based on this comment line.

I propose to create short design-document about the vision of error handling to make it easier to follow in all the patches.

We can adjust the behavior in a follow-up patch. (For example: instead of propagating the stack trace we can improve the insight tool to help to find the real error without propagation).

@anuengineer
Copy link
Contributor

https://issues.apache.org/jira/browse/HDDS-2175?focusedCommentId=16941392&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16941392

This comment is perhaps more relevant. Yes, let us create a doc that deals with this issue.

@elek
Copy link
Member

elek commented Nov 8, 2019

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