ColumnFamilyStore: only quote table names if necessary #230
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In a recent patch, we added quoting of table names to ColumnFamilyStore, to solve a bug where the existance of a table with a colon in its name broke various JMX requests. I was under the impression that these quoted names were only used internally, for checking the validity of table names, and the code unquoted them when necessary. I thought based on my not-extensive-enough testing that nodetool now works correctly both when table names with colon names exist, and when they don't. Unfortunately, further runs of all nodetool dtests revealed that the quoting broke the "nodetool getsstables" command (and only it). For some reason I don't fully understand yet, this specific command ends up using ColumnFamilyStore and getting confused by the quoted table names. So in this patch, ColumnFamilyStore makes an effort to only quote a table name if it's necessary (i.e., contains a colon), and only unquote it when necessary. After this patch, the tests for "nodetool tablestats" and "nodetool info" with a colon in a table's name continue to pass (these are the cases solved by the previous patches, and this patch doesn't break them), and the "nodetool getsstables" test starts working again. Signed-off-by: Nadav Har'El <nyh@scylladb.com>
|
Hi @denesb, this is a followup to the fix in #227 (it's a followup, meaning I assume you didn't revert that older patch). I'm sorry about the mess. I confirmed that after this patch, the two dtests that failed the previous CI run now pass. I also confirmed that the two uncommitted dtests fixed by 227 didn't break by this patch. I didn't run all dtests again (life is too short), but I'm hopefull. |
|
Submodule update PR: scylladb/scylladb#16146. |
nyh
added a commit
that referenced
this pull request
Dec 5, 2023
In a recent patch, we added quoting of table names to ColumnFamilyStore, to solve a bug where the existance of a table with a colon in its name broke various JMX requests. I was under the impression that these quoted names were only used internally, for checking the validity of table names, and the code unquoted them when necessary. I thought based on my not-extensive-enough testing that nodetool now works correctly both when table names with colon names exist, and when they don't. Unfortunately, further runs of all nodetool dtests revealed that the quoting broke the "nodetool getsstables" command (and only it). For some reason I don't fully understand yet, this specific command ends up using ColumnFamilyStore and getting confused by the quoted table names. So in this patch, ColumnFamilyStore makes an effort to only quote a table name if it's necessary (i.e., contains a colon), and only unquote it when necessary. After this patch, the tests for "nodetool tablestats" and "nodetool info" with a colon in a table's name continue to pass (these are the cases solved by the previous patches, and this patch doesn't break them), and the "nodetool getsstables" test starts working again. Signed-off-by: Nadav Har'El <nyh@scylladb.com> Closes: #230 (cherry picked from commit e9b2644)
nyh
added a commit
that referenced
this pull request
Dec 5, 2023
In a recent patch, we added quoting of table names to ColumnFamilyStore, to solve a bug where the existance of a table with a colon in its name broke various JMX requests. I was under the impression that these quoted names were only used internally, for checking the validity of table names, and the code unquoted them when necessary. I thought based on my not-extensive-enough testing that nodetool now works correctly both when table names with colon names exist, and when they don't. Unfortunately, further runs of all nodetool dtests revealed that the quoting broke the "nodetool getsstables" command (and only it). For some reason I don't fully understand yet, this specific command ends up using ColumnFamilyStore and getting confused by the quoted table names. So in this patch, ColumnFamilyStore makes an effort to only quote a table name if it's necessary (i.e., contains a colon), and only unquote it when necessary. After this patch, the tests for "nodetool tablestats" and "nodetool info" with a colon in a table's name continue to pass (these are the cases solved by the previous patches, and this patch doesn't break them), and the "nodetool getsstables" test starts working again. Signed-off-by: Nadav Har'El <nyh@scylladb.com> Closes: #230 (cherry picked from commit e9b2644) (cherry picked from commit f45067f)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In a recent patch, we added quoting of table names to ColumnFamilyStore, to solve a bug where the existance of a table with a colon in its name broke various JMX requests.
I was under the impression that these quoted names were only used internally, for checking the validity of table names, and the code unquoted them when necessary. I thought based on my not-extensive-enough testing that nodetool now works correctly both when table names with colon names exist, and when they don't.
Unfortunately, further runs of all nodetool dtests revealed that the quoting broke the "nodetool getsstables" command (and only it). For some reason I don't fully understand yet, this specific command ends up using ColumnFamilyStore and getting confused by the quoted table names.
So in this patch, ColumnFamilyStore makes an effort to only quote a table name if it's necessary (i.e., contains a colon), and only unquote it when necessary.
After this patch, the tests for "nodetool tablestats" and "nodetool info" with a colon in a table's name continue to pass (these are the cases solved by the previous patches, and this patch doesn't break them), and the "nodetool getsstables" test starts working again.