roachprod: remove support for spinning up cassandra clusters #64029
Conversation
|
Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR. My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan. |
|
This change is |
alan-mas
force-pushed
the
cassandra-refactor
branch
from
6db1de2 to
f16d6c2
Compare
|
Thank you for updating your pull request. My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI. I have added a few people who may be able to assist in reviewing:
🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan. |
There was a problem hiding this comment.
I think we could safely remove cassandra.go and everything that depends on it. I doubt anyone has used it in a long time.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @jlinder)
alan-mas
force-pushed
the
cassandra-refactor
branch
from
f16d6c2 to
6407ab7
Compare
|
Thank you for updating your pull request. My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan. |
alan-mas
force-pushed
the
cassandra-refactor
branch
from
6407ab7 to
e775292
Compare
There was a problem hiding this comment.
Suggestion for the commit message (and the PR title + description):
roachprod: remove support for spinning up cassandra clusters
When working on #47567, we discovered that the cassandra cluster support is no longer used and can be removed. This commit removes cassandra cluster support.
Reviewed 5 of 5 files at r1.
Reviewable status:
pkg/cmd/roachprod/main.go, line 173 at r1 (raw file):
} } case "cassandra":
With removing cassandra as a cluster type, there is no need for the switch clusterType statement and no need for the clusterType field (defined on about line 90 above).
pkg/testutils/lint/lint_test.go, line 215 at r1 (raw file):
stream.GrepNot(`cassandra\.go`), stream.GrepNot(`cassandra_yaml\.go`),
Why are these lines necessary?
pkg/testutils/lint/lint_test.go, line 1650 at r1 (raw file):
// TODO (alanmas): Remove this line after refactoring ssh.go // as now it is produccing "func is unused" errors. stream.GrepNot(`pkg/cmd/roachprod/ssh/ssh.go`),
I'd recommend just removing the functions that are unused from pkg/cmd/roachprod/ssh/ssh.go in this PR.
alan-mas
changed the title
alan-mas
force-pushed
the
cassandra-refactor
branch
from
e775292 to
b4c6262
Compare
If I do not add those lines go lint test failed as it goes and checks if the file exists: It might be a better way to avoid this issue that I am missing here. @jlinder This is also happening with |
There was a problem hiding this comment.
Reviewable status:
pkg/testutils/lint/lint_test.go, line 215 at r1 (raw file):
Previously, alan-mas (alanmas) wrote…
If I do not add those lines go lint test failed as it goes and checks if the file exists:
=== CONT TestLint/TestCopyrightHeaders lint_test.go:229: open /Users/admin/go/src/github.com/cockroachdb/cockroach/pkg/cmd/roachprod/install/cassandra.go: no such file or directory lint_test.go:229: open /Users/admin/go/src/github.com/cockroachdb/cockroach/pkg/cmd/roachprod/install/cassandra_yaml.go: no such file or directoryIt might be a better way to avoid this issue that I am missing here. @jlinder
This is also happening with
pkg/cmd/roachprod/ssh/ssh.goas we are changing his name toio.gowith the refactor.
I don't know much about how this linter works, but this does not seem like the right fix. Removing files should not make the linter fail. And this particular linter test is for validating files contain the correct copyright headers. My guess would be there is an ordering problem here. Let's look at this together.
alan-mas
force-pushed
the
cassandra-refactor
branch
3 times, most recently
from
4d22975 to
192a6dd
Compare
Just took away the The ones related to |
There was a problem hiding this comment.
On the cassandra lines: excellent.
On the ssh.go line, please see my comments in the line in lint_test.go.
Reviewed 2 of 2 files at r2.
Reviewable status:
pkg/testutils/lint/lint_test.go, line 1650 at r1 (raw file):
Previously, jlinder (James H. Linder) wrote…
I'd recommend just removing the functions that are unused from
pkg/cmd/roachprod/ssh/ssh.goin this PR.
The intent of TestStaticCheck is to tell the developer when code is no longer used so it can be removed. This helps us keep the code base clean. And it's particularly helpful for PRs like this one where we are removing code so we can be sure we remove all the related unused code too. Thus, this PR should remove any functions in pkg/cmd/roachprod/ssh/ssh.go that are no longer in use.
alan-mas
force-pushed
the
cassandra-refactor
branch
6 times, most recently
from
b605835 to
7ba1b4a
Compare
alan-mas
force-pushed
the
cassandra-refactor
branch
5 times, most recently
from
5ee238c to
32c1544
Compare
alan-mas
force-pushed
the
cassandra-refactor
branch
2 times, most recently
from
1631e5f to
355a5d1
Compare
Second part of cockroachdb#47567 where we need to ensure no dependency is getting affected when we remove ssh.go. So we are removing cassandra.go and all its dependencies. Release note: None
There was a problem hiding this comment.
I rebased the branch manually and fixed up the vendor update for Alan.
I think the PR is fine now. @jlinder do you agree?
I think if we agree it's fine we should merge this soon to prevent another vendor bump from "under" the PR, which would require one of us to re-do the vendor thing again.
Reviewed 3 of 5 files at r1, 1 of 2 files at r2, 4 of 4 files at r3, 2 of 2 files at r4.
Reviewable status:
There was a problem hiding this comment.
Yes, this is ready.
Reviewable status:
|
bors r+ |
|
Thank you for your help with this @knz! |
|
Build failed (retrying...): |
|
Build succeeded: |
The following install targets are removed in this commit: * `cassandra`: this should have been removed along with cockroachdb#64029. * `charybdefs`: this is also no longer used and should have been removed along with cockroachdb#102492. * `confluent`: only cdc tests would use this, and they implement their own logic to install `confluent`. In the future, we can consider including that logic in roachprod itself as part of `Install`, but that can be done in the future separately. * `tools`: this target doesn't work, which indicates no one has been using it. Epic: none Release note: None
The following install targets are removed in this commit: * `cassandra`: this should have been removed along with #64029. * `charybdefs`: this is also no longer used and should have been removed along with #102492. * `confluent`: only cdc tests would use this, and they implement their own logic to install `confluent`. In the future, we can consider including that logic in roachprod itself as part of `Install`, but that can be done in the future separately. * `tools`: this target doesn't work, which indicates no one has been using it. Epic: none Release note: None
The following install targets are removed in this commit: * `cassandra`: this should have been removed along with cockroachdb#64029. * `charybdefs`: this is also no longer used and should have been removed along with cockroachdb#102492. * `confluent`: only cdc tests would use this, and they implement their own logic to install `confluent`. In the future, we can consider including that logic in roachprod itself as part of `Install`, but that can be done in the future separately. * `tools`: this target doesn't work, which indicates no one has been using it. Epic: none Release note: None
When working on #47567, we discovered that the cassandra cluster support is no longer used and can be removed. This commit removes cassandra cluster support.
Release note: None