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.
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
Create Cassandra db schema on session initialization #5922
Create Cassandra db schema on session initialization #5922
Changes from 20 commits
5041f31
30db170
ce0c375
810ab1c
0d6383f
207945f
985f65b
1c30503
e4ab709
c329bba
e3c6045
492e15e
44c39dc
dfc0c43
cb8ae19
c3d0fbd
728a139
1b6683d
ce11cc1
de1c563
edabe22
d0e1976
d8479b5
02b6159
73d276a
57349a8
84b52e1
9c2f05b
2c8de88
eeb1951
601365d
4aeaad7
0167cb6
07b99da
04ec76f
7246494
d8613f1
a7853ec
10bd9aa
a682db2
35c26b5
ddf4fc0
bbf3ac8
a8e3aae
a46bf37
c2b89e0
de2d2ef
100208c
37d34dd
8d65894
23a9b62
d925074
154deb9
a238455
cc9db9c
9d8ba06
b0ac38a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check failure on line 180 in pkg/cassandra/config/config.go
GitHub Actions / lint
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.
check error
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.
actually, all this code effectively repeats
NewSession
, the only difference is the keyspace. Can you move it into a helpercreateSession(keyspace string)
?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.
The current implementation of
newSessionPrerequisites
kind of uses a hack to overridecluster.Keyspace
which was set inc.NewCluster()
, so that the connection can be established without keyspace.So, you are suggesting enclosing this override logic in
createSession(keyspace string)
?Or something like I create a copy of the
Configuration
with different keyspace to do the work, so that we don't setcluster.keyspace = ""
after creating it?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.
You can make the new function to depend on the config argument, and pass whichever config is needed in each instance, including cloning it to override keyspace
Check warning on line 187 in pkg/cassandra/config/config.go
Codecov / codecov/patch
pkg/cassandra/config/config.go#L180-L187
Check warning on line 192 in pkg/cassandra/config/config.go
Codecov / codecov/patch
pkg/cassandra/config/config.go#L189-L192
Check warning on line 199 in pkg/cassandra/config/config.go
Codecov / codecov/patch
pkg/cassandra/config/config.go#L198-L199
Check failure on line 276 in pkg/cassandra/config/config.go
GitHub Actions / lint
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 do we need custom functions, can the conditions not be expressed directly via tags?
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 we must have custom logic then just write it directly, with proper error messages - right now you are just returning a Boolean, so at best validator can say "property x did not validate".
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.
Couldn't find such validations here: https://github.com/asaskevich/govalidator
Fixed it.
Check warning on line 280 in pkg/cassandra/config/config.go
Codecov / codecov/patch
pkg/cassandra/config/config.go#L279-L280
Check failure on line 285 in pkg/cassandra/config/config.go
GitHub Actions / lint
Check warning on line 289 in pkg/cassandra/config/config.go
Codecov / codecov/patch
pkg/cassandra/config/config.go#L288-L289
Check warning on line 42 in pkg/cassandra/config/schema.go
Codecov / codecov/patch
pkg/cassandra/config/schema.go#L32-L42
Check warning on line 49 in pkg/cassandra/config/schema.go
Codecov / codecov/patch
pkg/cassandra/config/schema.go#L45-L49
Check warning on line 55 in pkg/cassandra/config/schema.go
Codecov / codecov/patch
pkg/cassandra/config/schema.go#L51-L55
Check warning on line 57 in pkg/cassandra/config/schema.go
Codecov / codecov/patch
pkg/cassandra/config/schema.go#L57
Check warning on line 71 in pkg/cassandra/config/schema.go
Codecov / codecov/patch
pkg/cassandra/config/schema.go#L60-L71
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.
it would make sense to trim spaces before checking for len=0.
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.
Done. Thanks
Check warning on line 74 in pkg/cassandra/config/schema.go
Codecov / codecov/patch
pkg/cassandra/config/schema.go#L73-L74
Check warning on line 77 in pkg/cassandra/config/schema.go
Codecov / codecov/patch
pkg/cassandra/config/schema.go#L77
Check warning on line 89 in pkg/cassandra/config/schema.go
Codecov / codecov/patch
pkg/cassandra/config/schema.go#L80-L89
Check warning on line 94 in pkg/cassandra/config/schema.go
Codecov / codecov/patch
pkg/cassandra/config/schema.go#L92-L94
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 would do it in generateSchemaIfNotPresent against
casQueries
, not here.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 didn't quite get this. The logic here is that if
queryString > 0
, means that there is a query string which does not ends with;
, that's what I am validating here.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.
ok then please make the error message self-explanatory
Check warning on line 96 in pkg/cassandra/config/schema.go
Codecov / codecov/patch
pkg/cassandra/config/schema.go#L96
Check warning on line 104 in pkg/cassandra/config/schema.go
Codecov / codecov/patch
pkg/cassandra/config/schema.go#L99-L104
Check warning on line 106 in pkg/cassandra/config/schema.go
Codecov / codecov/patch
pkg/cassandra/config/schema.go#L106
Check warning on line 113 in pkg/cassandra/config/schema.go
Codecov / codecov/patch
pkg/cassandra/config/schema.go#L109-L113
Check warning on line 118 in pkg/cassandra/config/schema.go
Codecov / codecov/patch
pkg/cassandra/config/schema.go#L115-L118
Check warning on line 123 in pkg/cassandra/config/schema.go
Codecov / codecov/patch
pkg/cassandra/config/schema.go#L120-L123
Check warning on line 127 in pkg/cassandra/config/schema.go
Codecov / codecov/patch
pkg/cassandra/config/schema.go#L125-L127
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.
There are too many functions in this file that are polluting the overall package namespace. I would prefer to introduce a helper struct
and define those functions on that struct (and minimize parameter passing)
Check warning on line 134 in pkg/cassandra/config/schema.go
Codecov / codecov/patch
pkg/cassandra/config/schema.go#L130-L134
Check warning on line 139 in pkg/cassandra/config/schema.go
Codecov / codecov/patch
pkg/cassandra/config/schema.go#L136-L139
Check warning on line 142 in pkg/cassandra/config/schema.go
Codecov / codecov/patch
pkg/cassandra/config/schema.go#L142