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

Discover dependencies table version automatically #1364

Merged
merged 5 commits into from
Feb 21, 2019

Conversation

black-adder
Copy link
Contributor

@black-adder black-adder commented Feb 20, 2019

Signed-off-by: Won Jun Jang wjang@uber.com

Resolves #1363

  • Deprecate EnableDependenciesV2 flag

Signed-off-by: Won Jun Jang <wjang@uber.com>
Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

there's one downside to this - during migration when the table is created but data is not copied, if the query-service is restarted it would switch to v2. But I don't know if there's a better way.

// GetDependencyVersion attempts to determine the version of the dependencies table.
// TODO: Remove this once we've migrated to V2 permanently.
func GetDependencyVersion(s cassandra.Session) Version {
if err := s.Query("SELECT ts from dependencies_v2 limit 1;").Exec(); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

what kind of error is returned? just curious

Copy link
Contributor Author

Choose a reason for hiding this comment

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

InvalidRequest: Error from server: code=2200 [Invalid query] message="unconfigured table dependencies"

Signed-off-by: Won Jun Jang <wjang@uber.com>
@codecov
Copy link

codecov bot commented Feb 20, 2019

Codecov Report

Merging #1364 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1364   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files         163     164    +1     
  Lines        7374    7376    +2     
======================================
+ Hits         7374    7376    +2
Impacted Files Coverage Δ
plugin/storage/cassandra/factory.go 100% <100%> (ø) ⬆️
...gin/storage/cassandra/dependencystore/bootstrap.go 100% <100%> (ø)
plugin/storage/cassandra/options.go 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0c9a2c9...cf61468. Read the comment docs.

Signed-off-by: Won Jun Jang <wjang@uber.com>
@pavolloffay
Copy link
Member

@black-adder I have raised this also because of jaegertracing/spark-dependencies#58. There can be other components which expect schema for v1 dependencies. We have failing CI jobs now in opearator bc dep. job tries to write to old dependnecies table.

in spark job we should do the same - automatically detect the table

)

// GetDependencyVersion attempts to determine the version of the dependencies table.
// TODO: Remove this once we've migrated to V2 permanently.
Copy link
Member

Choose a reason for hiding this comment

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

Add a pointer to issue

Signed-off-by: Won Jun Jang <wjang@uber.com>
@pavolloffay
Copy link
Member

@black-adder can this be merged?

@black-adder black-adder merged commit e895973 into master Feb 21, 2019
@black-adder black-adder deleted the discover_dependencies_table_automatically branch February 21, 2019 16:30
@ghost ghost removed the review label Feb 21, 2019
annanay25 pushed a commit to annanay25/jaeger that referenced this pull request Mar 1, 2019
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.

3 participants