Skip to content
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

Support UnrecoverableException #898

Merged
merged 2 commits into from
Nov 15, 2024

Conversation

noCharger
Copy link
Collaborator

@noCharger noCharger commented Nov 13, 2024

Description

Improve entry point exception handling by:

  1. Support rethrow UnrecoverableException and record emr job failure
  2. Switch general exception to throwable

Check List

  • Implemented unit tests
  • Implemented tests for combination with other commands
  • New added source code should include a copyright header
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@noCharger noCharger self-assigned this Nov 13, 2024
@noCharger noCharger force-pushed the unrecoverable branch 2 times, most recently from c89e27a to b66aac0 Compare November 14, 2024 10:51
@noCharger noCharger marked this pull request as ready for review November 14, 2024 10:53
* Represents an unrecoverable exception in session management and statement execution. This
* exception is used for errors that cannot be handled or recovered from.
*/
class UnrecoverableException private (message: String, cause: Throwable)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need to hide constructor?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why do we need to hide constructor?

Using "factory method pattern" is a common practice in Scala

  1. Readability: Using UnrecoverableException(cause) is often more readable than new UnrecoverableException(cause).
  2. Immutability: It can help enforce immutability by preventing direct instantiation of the class.
  3. Subclassing control: By making the constructor private, you prevent other classes from directly subclassing UnrecoverableException.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think factory pattern is normally used when we want to create instance of different classes for the same interface based on the parameters. (In this case, if we create different type of instance for UnrecoverableException, it makes more sense).

For 3, do we have reason to avoid subclassing of UnrecoverableException?

Copy link
Collaborator Author

@noCharger noCharger Nov 14, 2024

Choose a reason for hiding this comment

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

I think factory pattern is normally used when we want to create instance of different classes for the same interface based on the parameters. (In this case, if we create different type of instance for UnrecoverableException, it makes more sense).

factory method / static factory method is much simpler than the full-fledged Factory Pattern—still valuable, but primarily used to hide constructor complexity and ensure uniform creation rather than managing multiple types.
Users of the class can directly call UnrecoverableException(...) without needing to understand which specific constructors are available.

For 3, do we have reason to avoid subclassing of UnrecoverableException?

You aren't gonna need it

Signed-off-by: Louis Chu <[email protected]>
@noCharger noCharger merged commit 4f58bc8 into opensearch-project:main Nov 15, 2024
4 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Nov 15, 2024
* Support UnrecoverableException

Signed-off-by: Louis Chu <[email protected]>

* Add UT and IT

Signed-off-by: Louis Chu <[email protected]>

---------

Signed-off-by: Louis Chu <[email protected]>
(cherry picked from commit 4f58bc8)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
opensearch-trigger-bot bot pushed a commit that referenced this pull request Nov 15, 2024
* Support UnrecoverableException

Signed-off-by: Louis Chu <[email protected]>

* Add UT and IT

Signed-off-by: Louis Chu <[email protected]>

---------

Signed-off-by: Louis Chu <[email protected]>
(cherry picked from commit 4f58bc8)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
noCharger pushed a commit that referenced this pull request Nov 15, 2024
* Support UnrecoverableException



* Add UT and IT



---------


(cherry picked from commit 4f58bc8)

Signed-off-by: Louis Chu <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
noCharger pushed a commit that referenced this pull request Nov 15, 2024
* Support UnrecoverableException



* Add UT and IT



---------


(cherry picked from commit 4f58bc8)

Signed-off-by: Louis Chu <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
kenrickyap pushed a commit to Bit-Quill/opensearch-spark that referenced this pull request Dec 11, 2024
* Support UnrecoverableException

Signed-off-by: Louis Chu <[email protected]>

* Add UT and IT

Signed-off-by: Louis Chu <[email protected]>

---------

Signed-off-by: Louis Chu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants