-
Notifications
You must be signed in to change notification settings - Fork 277
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
Publish new name on table rename event #593
Conversation
79bacb7
to
80cb5a3
Compare
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.
lgtm
@@ -51,8 +55,10 @@ public RenameTableMessage( | |||
@JsonProperty("timestamp") final long timestamp, | |||
@JsonProperty("requestId") final String requestId, | |||
@JsonProperty("name") final String name, | |||
@JsonProperty("payload") final UpdatePayload<TableDto> payload | |||
@JsonProperty("newName") final String newName, | |||
@Nullable @JsonProperty("payload") final UpdatePayload<TableDto> payload |
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.
Wondering why do we want to have the payload be Nullable?
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.
In case the initial send fails, for example if full payload causes message to be too long, then the fallback is send the message again except null payload.
return new RenameTableMessage( | ||
id, | ||
timestamp, | ||
requestId, | ||
name.toString(), | ||
newName == null ? "" : newName.toString(), |
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.
Why not set it to null if null?
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.
Setting to null instead of empty string if null would be fine as well. If defaulting to null instead of empty string here helps on the client side then we can make that change.
Publish new name on table rename event, and add publish of new name in case of SNS message send failure as well. This allows clients to properly be notified of new name after rename event even in case of initial send failure.