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

fix: query id for TERMINATE should be case insensitive #5005

Conversation

big-andy-coates
Copy link
Contributor

@big-andy-coates big-andy-coates commented Apr 6, 2020

Description

fixes: #5003

The query id passed to TERMINATE <queryId> is currently case-sensitive. So, for example,

TERMINATE CSAS_A_0;

May work, but:

TERMINATE csas_a_0;

Won't.

CSAS and CTAS create queries with upper case ids. However, INSERT INTO creates mixed case queries, e.g. InsertQuery_3. Again, users must get the case right to terminate them.

SQL is meant to be case-insensitive by default! Hence this change switches terminate to be case-insensitive.

Testing done

Because the command topics of users will contain existing queries with query ids in upper and mixed case, we need to be a little careful any fix doesn't stop users from terminating historic queries or meaning restores leave previously terminated queries running. There is also another open PR that we need to ensure works with any change made here: #5002

To this end I went through the following steps, and ensured each worked, (this is also roughly the commit list):

  1. I first made QueryId internally case-insensitive, but externally case-preserving, i.e. a queryId of ID and id would generate the same hash code and return true from equals, but we return ID and id from toString. This is the least invasive fix. I ran both the master branch and feat: speed up restarts by not building topologies for terminated queries #5002 to ensure this wouldn't cause any issues elsewhere in the code.
  2. I updated RecoveryTest to have some terminates with the wrong case.
  3. (added some tests to QueryId)
  4. Changed the generation of queryIds to ensure they are always upper case - this means all new queries will have uppercase query ids. If all else fails, on the next breaking change release we can switch QueryId to be always uppercase.

It was not easily achievable to update QueryId to just uppercase its query id, as the text representation is used to, among other things, build the names for internal topics.

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 #")

@big-andy-coates big-andy-coates requested a review from a team as a code owner April 6, 2020 13:57
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.

lgtm

(This will allow us to make queryIds fully case-insensitive (as a breaking change) at version v1.0.
Copy link
Member

@JimGalasyn JimGalasyn left a comment

Choose a reason for hiding this comment

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

LGTM

@big-andy-coates big-andy-coates merged commit 588c1e9 into confluentinc:master Apr 7, 2020
@big-andy-coates big-andy-coates deleted the case_insensitive_terminate branch April 7, 2020 11:04
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.

TERMINATE should not be case sensitive
3 participants