You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zeppelin.apache.org by GitBox <gi...@apache.org> on 2021/01/22 03:31:38 UTC
[GitHub] [zeppelin] zjffdu opened a new pull request #4031: [ZEPPELIN-5215]. Use TApplicationException instead of TException to propagate exception to zeppelin server
zjffdu opened a new pull request #4031:
URL: https://github.com/apache/zeppelin/pull/4031
### What is this PR for?
Currently we use TException in RemoteInterpreterServer which make zeppelin server unknown what kind of error happens in interpreter process side. Using TApplicationException can propagate the exception to zeppelin server side.
See https://livebook.manning.com/book/programmers-guide-to-apache-thrift/chapter-9/87
### What type of PR is it?
[Improvement ]
### Todos
* [ ] - Task
### What is the Jira issue?
* https://issues.apache.org/jira/browse/ZEPPELIN-5215
### How should this be tested?
* Manually tested
### Screenshots (if appropriate)
Before
After
### Questions:
* Does the licenses files need update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? No
----------------------------------------------------------------
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] [zeppelin] zjffdu commented on pull request #4031: [ZEPPELIN-5215]. Use user defined exception instead of TException to propagate exception to zeppelin server
Posted by GitBox <gi...@apache.org>.
zjffdu commented on pull request #4031:
URL: https://github.com/apache/zeppelin/pull/4031#issuecomment-770156728
Will merge if no more comment
----------------------------------------------------------------
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] [zeppelin] Reamer commented on pull request #4031: [ZEPPELIN-5215]. Use user defined exception instead of TException to propagate exception to zeppelin server
Posted by GitBox <gi...@apache.org>.
Reamer commented on pull request #4031:
URL: https://github.com/apache/zeppelin/pull/4031#issuecomment-781127997
Yes, of course it can be done later. It was just a thought.
----------------------------------------------------------------
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] [zeppelin] zjffdu closed pull request #4031: [ZEPPELIN-5215]. Use user defined exception instead of TException to propagate exception to zeppelin server
Posted by GitBox <gi...@apache.org>.
zjffdu closed pull request #4031:
URL: https://github.com/apache/zeppelin/pull/4031
----------------------------------------------------------------
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] [zeppelin] asfgit closed pull request #4031: [ZEPPELIN-5215]. Use user defined exception instead of TException to propagate exception to zeppelin server
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #4031:
URL: https://github.com/apache/zeppelin/pull/4031
----------------------------------------------------------------
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] [zeppelin] Reamer commented on pull request #4031: [ZEPPELIN-5215]. Use user defined exception instead of TException to propagate exception to zeppelin server
Posted by GitBox <gi...@apache.org>.
Reamer commented on pull request #4031:
URL: https://github.com/apache/zeppelin/pull/4031#issuecomment-776575275
LGTM
I am not sure if it makes sense to distinguish between an exception thrown by the remote interpreter and an exception thrown by the Zeppelin server for the remote interpreter.
What do you think?
----------------------------------------------------------------
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] [zeppelin] zjffdu commented on pull request #4031: [ZEPPELIN-5215]. Use user defined exception instead of TException to propagate exception to zeppelin server
Posted by GitBox <gi...@apache.org>.
zjffdu commented on pull request #4031:
URL: https://github.com/apache/zeppelin/pull/4031#issuecomment-765485575
@Reamer That's make sense, I have updated the PR with user defined exception `InterpreterRPCException`
----------------------------------------------------------------
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] [zeppelin] Reamer commented on pull request #4031: [ZEPPELIN-5215]. Use TApplicationException instead of TException to propagate exception to zeppelin server
Posted by GitBox <gi...@apache.org>.
Reamer commented on pull request #4031:
URL: https://github.com/apache/zeppelin/pull/4031#issuecomment-765244322
I think we should use custom exceptions.
> User code operates at a higher layer and doesn’t generate TApplicationExceptions, rather making use of user-defined exceptions, which we cover next. TApplicationExceptions involve problems such as calling a method that isn’t implemented or failing to provide the necessary arguments to a method.
> Like TApplicationExceptions, user-defined exceptions are derived from TException. The distinction is that TApplicationExceptions are raised by the Apache Thrift framework and user-defined exceptions are raised by user code.
Other link for the same book: https://freecontent.manning.com/apache-thrift-handling-exceptions/
----------------------------------------------------------------
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] [zeppelin] zjffdu commented on pull request #4031: [ZEPPELIN-5215]. Use user defined exception instead of TException to propagate exception to zeppelin server
Posted by GitBox <gi...@apache.org>.
zjffdu commented on pull request #4031:
URL: https://github.com/apache/zeppelin/pull/4031#issuecomment-765866889
> The user defined exception looks much better. Do you have any idea why each function also throws the parent `TException`?
That is auto generated by thrift.
----------------------------------------------------------------
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] [zeppelin] Reamer edited a comment on pull request #4031: [ZEPPELIN-5215]. Use user defined exception instead of TException to propagate exception to zeppelin server
Posted by GitBox <gi...@apache.org>.
Reamer edited a comment on pull request #4031:
URL: https://github.com/apache/zeppelin/pull/4031#issuecomment-771803075
@zjffdu Where is the code for this change?
----------------------------------------------------------------
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] [zeppelin] zjffdu commented on pull request #4031: [ZEPPELIN-5215]. Use user defined exception instead of TException to propagate exception to zeppelin server
Posted by GitBox <gi...@apache.org>.
zjffdu commented on pull request #4031:
URL: https://github.com/apache/zeppelin/pull/4031#issuecomment-776616554
> LGTM
> I am not sure if it makes sense to distinguish between an exception thrown by the remote interpreter and an exception thrown by the Zeppelin server for the remote interpreter.
> What do you think?
Do you mean the 2 exceptions here ? https://github.com/apache/zeppelin/pull/4031/files#diff-3a436f559ddff9ba7280b2617547f5cc9d759e7f4bc6f04304c7e7fe7e01c061R98
We would treat them differently, if it is due to Interpreter exception, then no retry, otherwise it would retry (because it may due to network issue)
----------------------------------------------------------------
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] [zeppelin] zjffdu commented on pull request #4031: [ZEPPELIN-5215]. Use user defined exception instead of TException to propagate exception to zeppelin server
Posted by GitBox <gi...@apache.org>.
zjffdu commented on pull request #4031:
URL: https://github.com/apache/zeppelin/pull/4031#issuecomment-781082697
How about doing it later ? @Reamer Personally, I prefer to introduce new things when we have clear purpose, otherwise it may lead over design.
----------------------------------------------------------------
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] [zeppelin] Reamer edited a comment on pull request #4031: [ZEPPELIN-5215]. Use user defined exception instead of TException to propagate exception to zeppelin server
Posted by GitBox <gi...@apache.org>.
Reamer edited a comment on pull request #4031:
URL: https://github.com/apache/zeppelin/pull/4031#issuecomment-771803075
@zjffdu Where is the code for this change?
----------------------------------------------------------------
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] [zeppelin] zjffdu commented on pull request #4031: [ZEPPELIN-5215]. Use user defined exception instead of TException to propagate exception to zeppelin server
Posted by GitBox <gi...@apache.org>.
zjffdu commented on pull request #4031:
URL: https://github.com/apache/zeppelin/pull/4031#issuecomment-770186900
No problem, take your time @Reamer
----------------------------------------------------------------
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] [zeppelin] Reamer commented on pull request #4031: [ZEPPELIN-5215]. Use user defined exception instead of TException to propagate exception to zeppelin server
Posted by GitBox <gi...@apache.org>.
Reamer commented on pull request #4031:
URL: https://github.com/apache/zeppelin/pull/4031#issuecomment-765549606
The user defined exception looks much better. Do you have any idea why each function also throws the parent `TException`?
----------------------------------------------------------------
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] [zeppelin] zjffdu commented on pull request #4031: [ZEPPELIN-5215]. Use user defined exception instead of TException to propagate exception to zeppelin server
Posted by GitBox <gi...@apache.org>.
zjffdu commented on pull request #4031:
URL: https://github.com/apache/zeppelin/pull/4031#issuecomment-776435769
Reopen it, it looks like I accidentally clean the previous change. @Reamer Please help review it .
----------------------------------------------------------------
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] [zeppelin] Reamer commented on pull request #4031: [ZEPPELIN-5215]. Use user defined exception instead of TException to propagate exception to zeppelin server
Posted by GitBox <gi...@apache.org>.
Reamer commented on pull request #4031:
URL: https://github.com/apache/zeppelin/pull/4031#issuecomment-776668466
> Do you mean the 2 exceptions here ? https://github.com/apache/zeppelin/pull/4031/files#diff-3a436f559ddff9ba7280b2617547f5cc9d759e7f4bc6f04304c7e7fe7e01c061R98
Yes I thing, that's the right location. My thoughts went in the direction of 3 Exception:
- a TException for network problems -> retry
- an "InterpreterRPCException" to indicate exceptions thrown by the interpreter
- a "ServerRPCException" to indicate exceptions thrown by the server.
At the moment the handling is the same, but it might be helpful in the future to make a difference here.
----------------------------------------------------------------
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] [zeppelin] Reamer commented on pull request #4031: [ZEPPELIN-5215]. Use user defined exception instead of TException to propagate exception to zeppelin server
Posted by GitBox <gi...@apache.org>.
Reamer commented on pull request #4031:
URL: https://github.com/apache/zeppelin/pull/4031#issuecomment-770170036
Please wait, I want to take a look at the rendering.
----------------------------------------------------------------
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] [zeppelin] zjffdu closed pull request #4031: [ZEPPELIN-5215]. Use user defined exception instead of TException to propagate exception to zeppelin server
Posted by GitBox <gi...@apache.org>.
zjffdu closed pull request #4031:
URL: https://github.com/apache/zeppelin/pull/4031
----------------------------------------------------------------
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] [zeppelin] Reamer commented on pull request #4031: [ZEPPELIN-5215]. Use user defined exception instead of TException to propagate exception to zeppelin server
Posted by GitBox <gi...@apache.org>.
Reamer commented on pull request #4031:
URL: https://github.com/apache/zeppelin/pull/4031#issuecomment-771819462
I have checked the rendering options and do not see any improvement at the moment.
----------------------------------------------------------------
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] [zeppelin] Reamer commented on pull request #4031: [ZEPPELIN-5215]. Use user defined exception instead of TException to propagate exception to zeppelin server
Posted by GitBox <gi...@apache.org>.
Reamer commented on pull request #4031:
URL: https://github.com/apache/zeppelin/pull/4031#issuecomment-771803075
@zjffdu Wo ist der Code für diese Änderung?
----------------------------------------------------------------
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] [zeppelin] zjffdu commented on pull request #4031: [ZEPPELIN-5215]. Use user defined exception instead of TException to propagate exception to zeppelin server
Posted by GitBox <gi...@apache.org>.
zjffdu commented on pull request #4031:
URL: https://github.com/apache/zeppelin/pull/4031#issuecomment-781228066
Thanks @Reamer will merge if no more comment
----------------------------------------------------------------
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] [zeppelin] Reamer commented on pull request #4031: [ZEPPELIN-5215]. Use user defined exception instead of TException to propagate exception to zeppelin server
Posted by GitBox <gi...@apache.org>.
Reamer commented on pull request #4031:
URL: https://github.com/apache/zeppelin/pull/4031#issuecomment-771803075
----------------------------------------------------------------
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