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

SNOW-1000828 Expose specific error messages as part of the channel invalidation exception #721

Merged
merged 5 commits into from
Mar 25, 2024

Conversation

sfc-gh-alhuang
Copy link
Contributor

When a channel is invalidated, the original SDK does not throw a visible enough error. This PR makes SDK show more specific details for user when INVALID_CHANNEL is thrown.

JIRA

@sfc-gh-alhuang sfc-gh-alhuang marked this pull request as ready for review March 21, 2024 21:13
@sfc-gh-alhuang sfc-gh-alhuang requested review from sfc-gh-tzhang and a team as code owners March 21, 2024 21:13
Copy link
Contributor

@sfc-gh-asen sfc-gh-asen left a comment

Choose a reason for hiding this comment

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

LGTM, just had a small nit about naming

@@ -82,14 +84,15 @@ void invalidateChannelIfSequencersMatch(
String schemaName,
String tableName,
String channelName,
Long channelSequencer) {
Long channelSequencer,
String invalidationCause) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: in some methods it is invalidationCause and others it becomes errorMessage, would it be possible to reuse the same name so it is easier to follow?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree that we should use invalidationCause or invalidationReason in the function signature, but sometimes a channel gets invalidated is because of an error, so it's ok to call it errorMessage for such cases when passing it into the function

Copy link
Contributor

@sfc-gh-tzhang sfc-gh-tzhang left a comment

Choose a reason for hiding this comment

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

Left some minor comments, otherwise LGTM, thanks!

@@ -37,7 +37,9 @@ void addChannel(SnowflakeStreamingIngestChannelInternal<T> channel) {
channels.put(channel.getName(), channel);
// Invalidate old channel if it exits to block new inserts and return error to users earlier
if (oldChannel != null) {
oldChannel.invalidate("removed from cache");
String errorMessage =
Copy link
Contributor

Choose a reason for hiding this comment

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

Removing from cache is not an error, maybe let's call it invalidationReason or invalidationCause?

@@ -657,7 +657,7 @@ String getBlobPath(Calendar calendar, String clientPrefix) {
*
* @param blobData list of channels that belongs to the blob
*/
<CD> void invalidateAllChannelsInBlob(List<List<ChannelData<CD>>> blobData) {
<CD> void invalidateAllChannelsInBlob(List<List<ChannelData<CD>>> blobData, String errorMessage) {
Copy link
Contributor

Choose a reason for hiding this comment

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

invalidationReason or invalidationCause instead of errorMessage?

Comment on lines 61 to 62
// The cause of channel invalidation
private String invalidationCause;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we make it clear in the comment that this is the latest cause that we see since a channel could get invalidated multiple times?

Copy link
Contributor

@sfc-gh-asen sfc-gh-asen left a comment

Choose a reason for hiding this comment

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

Thanks!

@sfc-gh-alhuang sfc-gh-alhuang merged commit 4d5f072 into master Mar 25, 2024
14 checks passed
@sfc-gh-alhuang sfc-gh-alhuang deleted the alhuang-channel-invalidation-message branch March 25, 2024 20:59
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.

5 participants