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

[FLINK-36094] CDC SchemaRegistryRequestHandler should throw exception which is not SchemaEvolveException #3558

Merged
merged 7 commits into from
Aug 22, 2024

Conversation

loserwang1024
Copy link
Contributor

As shown in https://issues.apache.org/jira/browse/FLINK-36094, Current, SchemaRegistryRequestHandler only throw
SchemaEvolveException, which will not handle the others(like network, oom, or else.

@loserwang1024
Copy link
Contributor Author

@leonardBang @yuxiqian @ruanhang1993 , CC, WDYT?

Copy link
Contributor

@yuxiqian yuxiqian left a comment

Choose a reason for hiding this comment

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

Thanks for @loserwang1024's quick fix, just left some minor comments.

Copy link
Contributor

@ruanhang1993 ruanhang1993 left a comment

Choose a reason for hiding this comment

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

In SchemaOperator, more places should be modified.
For example, line 540& 555 should be throw new IllegalStateException(xxx, e).

Could you please modify these parts together?

@loserwang1024
Copy link
Contributor Author

Could you please modify these parts together?

done it

Copy link
Contributor

@yuxiqian yuxiqian left a comment

Choose a reason for hiding this comment

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

Thanks for @loserwang1024's rapid response, LGTM

@@ -275,6 +277,8 @@ public void resetToCheckpoint(long checkpointId, @Nullable byte[] checkpointData
throw new IOException(
"Unrecognized serialization version " + schemaManagerSerializerVersion);
}
} catch (Throwable throwable) {
context.failJob(throwable);
Copy link
Contributor

Choose a reason for hiding this comment

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

why not re-throw this throwable here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

add it now

Copy link
Contributor

@leonardBang leonardBang left a comment

Choose a reason for hiding this comment

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

Thanks @loserwang1024 for the improvement and @yuxiqian for the review +1

@leonardBang leonardBang merged commit 6205a5a into apache:master Aug 22, 2024
21 checks passed
yuxiqian pushed a commit to yuxiqian/flink-cdc that referenced this pull request Aug 22, 2024
…equestHandler thrown

 This closes apache#3558.

(cherry picked from commit 6205a5a)
leonardBang pushed a commit to yuxiqian/flink-cdc that referenced this pull request Aug 27, 2024
…equestHandler thrown

 This closes apache#3558.

(cherry picked from commit 6205a5a)
leonardBang pushed a commit that referenced this pull request Aug 27, 2024
…equestHandler thrown

 This closes #3558.

(cherry picked from commit 6205a5a)
qiaozongmi pushed a commit to qiaozongmi/flink-cdc that referenced this pull request Sep 23, 2024
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.

4 participants