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

Add peers bootstrap merge local writes integration test #82

Merged
merged 7 commits into from
Sep 13, 2016

Conversation

robskillington
Copy link
Collaborator

@robskillington robskillington commented Sep 12, 2016

@coveralls
Copy link

coveralls commented Sep 12, 2016

Coverage Status

Coverage decreased (-0.2%) to 80.022% when pulling b3dd0d8 on merge-latest-bootstrapped-block into 36694a1 on master.

{"expected.log", &expectedDebugFilePath},
{"actual.log", &actualDebugFilePath},
} {
*entry.filePath = debugFilePathPrefix + "_" + entry.suffix
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: put the file operations in a function and call it on "expected.log" and "actual.log" separately? not sure if it worth to loop through 2 structs

Copy link
Collaborator Author

@robskillington robskillington Sep 12, 2016

Choose a reason for hiding this comment

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

This is just a test harness however? I'm trying to avoid adding another struct type.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will call another function though, guess it's not too much of a pollution.

@coveralls
Copy link

coveralls commented Sep 12, 2016

Coverage Status

Coverage decreased (-0.3%) to 79.888% when pulling 5db1e92 on merge-latest-bootstrapped-block into 36694a1 on master.

… anonymous struct for creating debug files
@coveralls
Copy link

coveralls commented Sep 12, 2016

Coverage Status

Coverage decreased (-0.2%) to 80.06% when pulling a684662 on merge-latest-bootstrapped-block into 36694a1 on master.

require.Equal(t, expected, actual)
}

func writeVerifyDebugOutput(t *testing.T, filePath string, start, end time.Time, series seriesList) {
w, err := os.OpenFile(filePath, os.O_APPEND|os.O_WRONLY, os.ModeAppend)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this get cleaned up later since it's append?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is an integration test component that is only enabled "when you want it". It's for viewing the difference between expected and actual datapoints that arrived at the destination node. It needs to live on after the test run is finished so you can do a diff between the two files.

multiErr := xerrors.NewMultiError()
mutex := sync.Mutex{}

Choose a reason for hiding this comment

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

nit: using var mutex sync.Mutex is probably more idiomatic

@coveralls
Copy link

coveralls commented Sep 13, 2016

Coverage Status

Coverage decreased (-0.2%) to 80.06% when pulling dde3c9c on merge-latest-bootstrapped-block into 36694a1 on master.

@xichen2020
Copy link

other than the nit LGTM

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 79.931% when pulling a522f51 on merge-latest-bootstrapped-block into 36694a1 on master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 79.931% when pulling a522f51 on merge-latest-bootstrapped-block into 36694a1 on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 80.067% when pulling 0f24588 on merge-latest-bootstrapped-block into 36694a1 on master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 80.067% when pulling 0f24588 on merge-latest-bootstrapped-block into 36694a1 on master.

cw9 pushed a commit that referenced this pull request Oct 16, 2018
prateek pushed a commit that referenced this pull request Apr 7, 2019
prateek pushed a commit that referenced this pull request Apr 8, 2019
jnyi added a commit to jnyi/m3 that referenced this pull request May 8, 2024
Revert "Merge pull request m3db#79 from databricks/double-write-bugs"
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.

5 participants