Navigation Menu

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

feat: Support IF EXISTS keyword on DROP CONNECTOR #6067

Merged
merged 2 commits into from Aug 24, 2020

Conversation

jzaralim
Copy link
Contributor

@jzaralim jzaralim commented Aug 20, 2020

Description

Fixes #5988

Added IF EXISTS keyword to the DROP CONNECTOR command.

Previously, this error message would have shown if the connector does not exist:

 Error                                   
-----------------------------------------
 {
  "error_code" : 404,
  "message" : "Connector FOO not found"
} 
-----------------------------------------

With this change, if the IF EXISTS keyword is used, the response is:

 Message                         
---------------------------------
 Connector 'FOO' does not exist. 
---------------------------------

If the IF EXISTS keyword is not used, the response is the original error response.

I added a new Entity class called WarningEntity that prints a message without the Error header. It could also be used in other similar situations, such as #5992.

Testing done

Added tests to DropConnectorExecutorTest.java

Reviewer checklist

  • Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
  • Ensure relevant issues are linked (description should include text like "Fixes #")

@jzaralim jzaralim requested review from JimGalasyn and a team as code owners August 20, 2020 20:16

@Override
public String toString() {
return "ErrorEntity{"
Copy link
Member

Choose a reason for hiding this comment

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

I think you mean WarningEntity here

Copy link
Member

@stevenpyzhang stevenpyzhang left a comment

Choose a reason for hiding this comment

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

lgtm, could you also post a sample CLI output in the PR description of what the warning response would look like?

return "DropConnector{"
+ "connectorName='" + connectorName + '\''
+ '}';
return toStringHelper(this)
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason why we're switching to using this toStringHelper function now? There seems to be a mix of using this method vs defining out manually what the string representation looks like for different AstNodes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not particularly, just thought it looked neater especially now that there is more than one field.

final ConnectResponse<String> response =
serviceContext.getConnectClient().delete(connectorName);

if (response.error().isPresent()) {
return Optional.of(new ErrorEntity(statement.getStatementText(), response.error().get()));
if (ifExists && response.httpCode() == 404) {
Copy link
Member

Choose a reason for hiding this comment

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

You may want to use HttpStatus.SC_NOT_FOUND instead of just 404.

Copy link
Member

@spena spena left a comment

Choose a reason for hiding this comment

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

Cool, LGTM. I just left a minor comment.

@jzaralim jzaralim merged commit 2c99df9 into confluentinc:master Aug 24, 2020
@jzaralim jzaralim deleted the issue-5988 branch August 24, 2020 21:58
sarwarbhuiyan pushed a commit to sarwarbhuiyan/ksql that referenced this pull request Sep 4, 2020
* feat: Support IF EXISTS keyword on DROP CONNECTOR

* fix: review comments

Co-authored-by: Zara Lim <zlim@zara-lim-mbp16.attlocal.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Query upgrades and migrations
  
Awaiting triage
Development

Successfully merging this pull request may close these issues.

Support IF EXISTS keyword on DROP CONNECTOR
3 participants