-
Notifications
You must be signed in to change notification settings - Fork 340
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[CDAP-21070] Introduce error details provider to get more information about exceptons from plugins #15718
Closed
Closed
[CDAP-21070] Introduce error details provider to get more information about exceptons from plugins #15718
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
41 changes: 41 additions & 0 deletions
41
cdap-api-common/src/main/java/io/cdap/cdap/api/exception/ErrorDetailsProvider.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
/* | ||
* Copyright © 2024 Cask Data, Inc. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); you may not | ||
* use this file except in compliance with the License. You may obtain a copy of | ||
* the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT | ||
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the | ||
* License for the specific language governing permissions and limitations under | ||
* the License. | ||
*/ | ||
|
||
package io.cdap.cdap.api.exception; | ||
|
||
import javax.annotation.Nullable; | ||
|
||
/** | ||
* Interface for providing error details. | ||
* | ||
* <p> | ||
* Implementations of this interface can be used to provide more detailed error information | ||
* for exceptions that occur within the code using {@link ProgramFailureException}. | ||
* </p> | ||
*/ | ||
public interface ErrorDetailsProvider<T> { | ||
|
||
/** | ||
* Returns a {@link RuntimeException} that wraps the given exception | ||
* with more detailed information. | ||
* | ||
* @param e the exception to wrap | ||
* @param conf configuration object | ||
* @return {@link RuntimeException} that wraps the given exception | ||
* with more detailed information. | ||
*/ | ||
RuntimeException getExceptionDetails(Throwable e, @Nullable T conf); | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not clear on why this interface is needed and why we want to pass the Configuration through it. Can you describe how it is intended to be used? Maybe include one example test plugin in the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This interface helps in cases where just generic exceptions like
IOException
are thrown from plugin but we want to generate further information from it. LikeGoogle APIs
throughGoogleJsonResponseException
, sogoogle-cloud
plugin classes can implement this interface, for example see PR: data-integrations/google-cloud#1450There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The intent of adding
Configuration
is for classes to load the delegate class and redirect the call to them. For example, in this caseStageTrackingInputFormat
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be the plugin doing it? Why it is part of the cdap-api contract that has to be invoked from the platform?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I didn't get your question exactly? Trying to give a shot at it:
Plugin is implementing the interface and extracting more details from it and then returning
ProgramFailureException
.cdap-api
just defines the interface.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the older PR: data-integrations/google-cloud#1445 without this interface. In every plugin we had to do all the wrapping stuff to throw the
ProgramFailureException
but now it just needs to implement this one method.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly, the idea is to try and consolidate the exception handling in a single place instead of in a bunch of places in each InputFormat/OutputFormat method?
I'm not sure that's actually what we want. Exceptions should generally be handled where they are thrown, otherwise a lot of context is lost and the logic is brittle. For example, some layer in between may wrap one of the exceptions and unknowingly break the exception handling. Also, a NOT_FOUND error in one part of the code (ex: writing to GCS) may be an error while a NOT_FOUND in another part of the code (ex: deleting a GCS object) should get ignored. Putting all the exception handling in a single place encourages broad general handling when it is often not a great solution.
It does sometimes make sense, like when we're setting up a HTTP server we register an Exception Handler class that handles very generic common logic like mapping various known exceptions into their corresponding http error codes, etc. For use cases like this it would be better to have an API for registering some exception handling class rather than having a method in the InputFormat/OutputFormat. Though again I'm not sure that's really what we want in this situation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are not trying to handle any exceptions, the behavior can be different in every plugin. What the method does is just wrapping the exception in a particular platform exception after extracting more information out of it.