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

roachtest: move the logger from cluster to test #31321

Merged
merged 3 commits into from Oct 16, 2018

Conversation

Projects
None yet
4 participants
@andreimatei
Member

andreimatei commented Oct 12, 2018

Before this patch, c.l was essentially a test's logger. But that logger
belongs better on a test than on a cluster, and so I've moved it.
c.l still exists, and it's being set at a high level to t.l. The idea is
that c.l will still be used, at least for the time being, by other
cluster methods, but otherwise tests will switch to using t.l instead of
c.l (this is being done in the next commit).

Release note: None

@cockroach-teamcity

This comment has been minimized.

Show comment
Hide comment
@cockroach-teamcity

cockroach-teamcity Oct 12, 2018

Member

This change is Reviewable

Member

cockroach-teamcity commented Oct 12, 2018

This change is Reviewable

@andreimatei

This comment has been minimized.

Show comment
Hide comment
@andreimatei

andreimatei Oct 12, 2018

Member

First commit is elsewhere.

Member

andreimatei commented Oct 12, 2018

First commit is elsewhere.

@andreimatei andreimatei requested a review from benesch Oct 16, 2018

@benesch

:lgtm: but I didn't look at the first commit

Reviewed 3 of 3 files at r2, 27 of 27 files at r3, 6 of 6 files at r4.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale)


pkg/cmd/roachtest/cluster.go, line 832 at r3 (raw file):

	// This cloned cluster is not taking ownership. The parent retains it.
	cpy.owned = false
	cpy.destroyed = nil

Is this in the right commit?


pkg/cmd/roachtest/test.go, line 919 at r2 (raw file):

			c = c.clone()
		}
		c.setTest(t)

Is this in the right commit?

@andreimatei

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained


pkg/cmd/roachtest/cluster.go, line 832 at r3 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

Is this in the right commit?

no, moved to the previous one. thanks


pkg/cmd/roachtest/test.go, line 919 at r2 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

Is this in the right commit?

yes. The setTest() call got moved below the clone() call; clone() used to take a test, but no longer.

@benesch

Reviewed 31 of 31 files at r5, 26 of 26 files at r6, 6 of 6 files at r7.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale)

andreimatei added some commits Oct 12, 2018

roachtest: move the logger from cluster to test
Before this patch, c.l was essentially a test's logger. But that logger
belongs better on a test than on a cluster, and so I've moved it.
c.l still exists, and it's being set at a high level to t.l. The idea is
that c.l will still be used, at least for the time being, by other
cluster methods, but otherwise tests will switch to using t.l instead of
c.l (this is being done in the next commit).

Release note: None
roachtest: tests switch to t.l from c.l
Now that t.l exists.

Release note: None
roachtest: remove the artifacts global
Recently we got better at plumbing the dirs where different log files
should go to through. This patch completes that migration by removing
the artifacts global.
A buglet is also fixed - the global was being used by the Jepsen tests
directly, in ignorance of the fact that, if --count is used, logs need
to go to a different dir for each iteration.

Release note: None
@andreimatei

bors r+

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale)

craig bot pushed a commit that referenced this pull request Oct 16, 2018

Merge #31321
31321: roachtest: move the logger from cluster to test r=andreimatei a=andreimatei

Before this patch, c.l was essentially a test's logger. But that logger
belongs better on a test than on a cluster, and so I've moved it.
c.l still exists, and it's being set at a high level to t.l. The idea is
that c.l will still be used, at least for the time being, by other
cluster methods, but otherwise tests will switch to using t.l instead of
c.l (this is being done in the next commit).

Release note: None

Co-authored-by: Andrei Matei <andrei@cockroachlabs.com>
@craig

This comment has been minimized.

Show comment
Hide comment
@craig

craig bot commented Oct 16, 2018

Build succeeded

@craig craig bot merged commit 6fb12f9 into cockroachdb:master Oct 16, 2018

3 checks passed

GitHub CI (Cockroach) TeamCity build finished
Details
bors Build succeeded
Details
license/cla Contributor License Agreement is signed.
Details

@andreimatei andreimatei deleted the andreimatei:roachtest-logger branch Oct 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment