-
Notifications
You must be signed in to change notification settings - Fork 56
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
pgsql ALTER TOPIC command to register new schema #1309
pgsql ALTER TOPIC command to register new schema #1309
Conversation
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.
Make sure to include negative scenario ITs to detect when ALTER TABLE etc is called with unsupported operation, not add
.
: ALTER opttable_alter_type (IF_P EXISTS)? relation_expr (alter_table_cmds | partition_cmd) | ||
| ALTER opttable_alter_type ALL IN_P TABLESPACE name (OWNED BY role_list)? SET TABLESPACE name opt_nowait |
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.
Seems they are listing out the possibles explicitly, why not just put ALTER TABLE
and ALTER TOPIC
here too?
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 thought since they are logically exactly the same the use like alias
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.
Understood, prefer we list them out explicitly if that's okay with you.
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.
Sure thing!
@@ -36,9 +36,9 @@ public SqlCreateStreamListener( | |||
this.tokens = tokens; | |||
} | |||
|
|||
public StreamInfo streamInfo() | |||
public Stream streamInfo() |
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 you want to have a suffix for these model class names, consider Model
instead of Info
.
Alternatively consider a prefix like PgsqlStream
.
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 actually removed suffixes from all other places this got missed
"${app}/alter.topic.unsupported.operation/client", | ||
"${app}/alter.topic.unsupported.operation/server" | ||
}) | ||
public void shouldNotAlterTopicColumn() throws Exception |
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.
public void shouldNotAlterTopicColumn() throws Exception | |
public void shouldNotAlterTopicModifyColumn() throws Exception |
?
"${app}/alter.topic.unsupported.operation/client", | ||
"${app}/alter.topic.unsupported.operation/server" |
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.
"${app}/alter.topic.unsupported.operation/client", | |
"${app}/alter.topic.unsupported.operation/server" | |
"${app}/alter.topic.modify.column.rejected/client", | |
"${app}/alter.topic.modify.column.rejected/server" |
throw new UnsupportedOperationException( | ||
String.format("Unsupported alter operation: %s", alterExpr.operation())); |
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 using exceptions for flow control, which we typically avoid because it is heavy.
"${pgsql}/alter.topic.unsupported.operation/client" | ||
}) | ||
public void shouldNotAlterTopicColumn() throws Exception |
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.
"${pgsql}/alter.topic.unsupported.operation/client" | |
}) | |
public void shouldNotAlterTopicColumn() throws Exception | |
"${pgsql}/alter.topic.modify.column.rejected/client" | |
}) | |
public void shouldNotAlterTopicModifyColumn() throws Exception |
Fixes #1058