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

Create new cassandra table "operation_names_v2" with "spanKind" column for operation name index #1937

Merged
merged 6 commits into from
Nov 22, 2019

Conversation

guo0693
Copy link
Contributor

@guo0693 guo0693 commented Nov 19, 2019

Signed-off-by: Jun Guo [email protected]

Which problem is this PR solving?

Short description of the changes

  • add migration script
  • read from the latest table if available, otherwise fail back to previous table

@@ -91,7 +91,7 @@ func TestCassandraFactory(t *testing.T) {
_, err = f.CreateArchiveSpanWriter()
assert.EqualError(t, err, "archive storage not configured")

f.archiveConfig = &mockSessionBuilder{}
f.archiveConfig = newMockSessionBuilder(session, nil)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have to pass the mock session so that operation names table check can pass

@codecov
Copy link

codecov bot commented Nov 19, 2019

Codecov Report

Merging #1937 into master will decrease coverage by 0.01%.
The diff coverage is 96.61%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1937      +/-   ##
==========================================
- Coverage   98.46%   98.44%   -0.02%     
==========================================
  Files         198      198              
  Lines        9744     9794      +50     
==========================================
+ Hits         9594     9642      +48     
- Misses        114      115       +1     
- Partials       36       37       +1
Impacted Files Coverage Δ
...gin/storage/cassandra/spanstore/operation_names.go 97.59% <96.61%> (-2.41%) ⬇️

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 115f931...ffae69d. Read the comment docs.

…n for operation name index

- add migration script
- read from the latest table if available, otherwise fail back to previous table

Signed-off-by: Jun Guo <[email protected]>
@guo0693
Copy link
Contributor Author

guo0693 commented Nov 19, 2019

Tested in local env, the code works with both old and new table

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.

Overall looks good, but I don't like this constant switching between versions. The differences between v1 and v2 should be encapsulated into tableMeta so that the rest of the code doesn't have to keep doing the switching.

plugin/storage/cassandra/spanstore/operation_names.go Outdated Show resolved Hide resolved
plugin/storage/cassandra/spanstore/operation_names.go Outdated Show resolved Hide resolved
plugin/storage/cassandra/spanstore/operation_names.go Outdated Show resolved Hide resolved
plugin/storage/cassandra/spanstore/operation_names.go Outdated Show resolved Hide resolved
plugin/storage/cassandra/spanstore/operation_names.go Outdated Show resolved Hide resolved
plugin/storage/cassandra/spanstore/operation_names.go Outdated Show resolved Hide resolved
plugin/storage/cassandra/spanstore/operation_names.go Outdated Show resolved Hide resolved
plugin/storage/cassandra/spanstore/operation_names_test.go Outdated Show resolved Hide resolved
plugin/storage/cassandra/spanstore/operation_names_test.go Outdated Show resolved Hide resolved
plugin/storage/cassandra/spanstore/operation_names_test.go Outdated Show resolved Hide resolved
plugin/storage/cassandra/spanstore/operation_names.go Outdated Show resolved Hide resolved
plugin/storage/cassandra/spanstore/operation_names.go Outdated Show resolved Hide resolved
plugin/storage/cassandra/spanstore/operation_names_test.go Outdated Show resolved Hide resolved
plugin/storage/cassandra/spanstore/operation_names_test.go Outdated Show resolved Hide resolved
} else {
assert.EqualError(t, err, "Error reading operation_names from storage: "+expErr.Error())
assert.EqualError(t, err, fmt.Sprintf("Error reading %s from storage: %s", schemas[test.schemaVersion].tableName, test.expErr.Error()))
Copy link
Member

Choose a reason for hiding this comment

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

arguments to assert functions are already treated as printf args, you don't need additional fmt.Sprintf

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried, but it failed.

func EqualError(t TestingT, theError error, errString string, msgAndArgs ...interface{}) bool
fmt.Sprintf is used to generate the errString, without fmt.Sprintf, it will treat "schemas[test.schemaVersion].tableName, test.expErr.Error()" as msg args for the output when the assert fail

plugin/storage/cassandra/spanstore/operation_names_test.go Outdated Show resolved Hide resolved
plugin/storage/cassandra/spanstore/operation_names.go Outdated Show resolved Hide resolved
{name: "test new schema without error", schemaVersion: latestVersion, expErr: nil},
{name: "test new schema with scan error", schemaVersion: latestVersion, expErr: scanError},
} {
t.Run(fmt.Sprintf("%s", test.name), func(t *testing.T) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have optimized to use t.Run(test.name, func) in the PR 1921-3. Try to avoid update it in 2nd branch and merge into 3rd branch again.

@yurishkuro
Copy link
Member

why is the build failing?

@guo0693
Copy link
Contributor Author

guo0693 commented Nov 22, 2019

why is the build failing?

Because the t.run is using fmt.Sprintf("%s", test.name), instead of test.name directly.

I fixed it in the PR 1921-3 when I merge this branch into 1921-3 last night before travis throw this error.

I don't want to change this branch and solve merge conflict again in 1921-3 since the build will pass when I merge 1921-3 to this branch.

My plan is:

  1. merge 1921-3 to this branch
  2. get the build pass
  3. merge this branch to master
  4. open PR for endpoint & query service changes against master
  5. open diff in internal repo to pull the latest master

@yurishkuro
Copy link
Member

We do not merge non-compiling PRs to master. Why is it so difficult to apply the t.Run to two lines? If it matches another branch, there won't be merge conflicts.

@guo0693
Copy link
Contributor Author

guo0693 commented Nov 22, 2019

We do not merge non-compiling PRs to master. Why is it so difficult to apply the t.Run to two lines? If it matches another branch, there won't be merge conflicts.

We are not going to merge non-compiling PRs to master.
1921-3 is going to be merged to 1921-2 (this feature branch) first so that this PR will pass the build. We will then merge this PR to master.

Both 1921-2 and 1921-3 touched the same files, therefore everytime I touch operation_names.go and test, I have to solve merge conflicts in 1921-3. I have repeated that several times yesterday, feel like it's not the right way.

In general, I want to propose a process for working on the big feature changes in the future:

  1. create a feature branch base on master
  2. open staggered PRs against the feature branch (e.g. PR1, PR2 base on PR1, PR3 base on PR2 etc) and have those small staggered PRs reviewed and merged into feature branch. We should not do code changes directly in feature branch.
  3. After all staggered PRs are merged into this feature branch, the feature branch is now complete and ready for merge. Merge feature branch into master, solve potential conflict with master if necessary at this stage.

What do you think?

In this case 1921-1 is already merged, we kind of missed the train for this feature. Next time we could use this methodology.

@guo0693
Copy link
Contributor Author

guo0693 commented Nov 22, 2019

If you are OK with above, can i get a stamp on guo0693#1 first?
I will merge it into this branch. Then you can stamp on this PR. I will then merge it into master.

@yurishkuro
Copy link
Member

I don't understand the workflow you're proposing. Merging something into this PR will require another top to bottom review, how does it make reviewing easier?

@guo0693
Copy link
Contributor Author

guo0693 commented Nov 22, 2019

I don't understand the workflow you're proposing. Merging something into this PR will require another top to bottom review, how does it make reviewing easier?

"We should not do code changes directly in feature branch." Each individual PR is reviewed separately. As long as there is no un-necessary code changes in feature branch (solving merged conflict in the last step is necessary code change, and the single last commit can be easily reviewed), we don't need another top to bottom review.

Here, we need to make sure:

  1. you are ok with 1921-3 PR, and you are ok with 1921-2 PR
  2. when we merge 1921-3 PR into 1921-2, there is no code changes.
  3. Merge 1921-2 PR into master. Do not add any more code changes in 1921-2.

@guo0693
Copy link
Contributor Author

guo0693 commented Nov 22, 2019

Btw, at the same time, I pushed changes to fix the travis build in this branch, also rebased 1921-3 onto this branch. You don't need to go over the 1921-3 from top to bottom, the rebase doesn't lead to any code changes.

It's probably easier to go over the workflow in person on a whiteboard when you are back :)

@yurishkuro yurishkuro merged commit 11cf2b9 into jaegertracing:master Nov 22, 2019
@guo0693 guo0693 deleted the #1921-2 branch November 27, 2019 23:22
@pavolloffay pavolloffay added this to the Release 1.16 milestone Dec 17, 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