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.

Sign up for GitHub

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

Jump to bottom

raft: raftlogger and rafttest should use pkg/util/log instead of stdlib log package #132262

Open
rickystewart opened this issue Oct 9, 2024 · 0 comments
Open

raft: raftlogger and rafttest should use pkg/util/log instead of stdlib log package #132262

rickystewart opened this issue Oct 9, 2024 · 0 comments
Labels
C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-kv KV Team

Comments

@rickystewart
Copy link
Collaborator

rickystewart commented Oct 9, 2024 •
edited
Loading

This lint failure was uncovered when #132258 was addressed:

        github.com/cockroachdb/cockroach/pkg/raft/raftlogger: log <- please use "util/log" instead of "log"

Attempting to follow this advice results in the following:

pkg/raft/raftlogger/logger.go:63:49: undefined: log.New
pkg/raft/raftlogger/logger.go:63:76: undefined: log.LstdFlags
pkg/raft/raftlogger/logger.go:64:49: undefined: log.New
pkg/raft/raftlogger/logger.go:75:7: undefined: log.Logger
pkg/raft/raftlogger/logger.go:89:5: l.Output undefined (type *DefaultLogger has no field or method Output)
pkg/raft/raftlogger/logger.go:95:5: l.Output undefined (type *DefaultLogger has no field or method Output)
pkg/raft/raftlogger/logger.go:108:4: l.Output undefined (type *DefaultLogger has no field or method Output)
pkg/raft/raftlogger/logger.go:112:4: l.Output undefined (type *DefaultLogger has no field or method Output)
pkg/raft/raftlogger/logger.go:124:4: l.Output undefined (type *DefaultLogger has no field or method Output)
pkg/raft/raftlogger/logger.go:129:4: l.Output undefined (type *DefaultLogger has no field or method Output)
pkg/raft/raftlogger/logger.go:129:4: too many errors

So some refactoring is necessary here.

For now we will exempt this in the lint check.

There is also an issue with rafttest (fails to find a function log.Printf())

Jira issue: CRDB-42906
@rickystewart rickystewart added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-kv KV Team labels Oct 9, 2024
rickystewart added a commit to rickystewart/cockroach that referenced this issue Oct 9, 2024
@rickystewart
lint: fix TestForbiddenImports
fbb1641 
We need to set `Dir` in the `packages.Load()` call or else the whole
thing doesn't work. It would be really nice if it threw an error instead
of simply doing nothing, but it is what it is. To guard against this in
the future I added an error if the list is empty.

Fix the test then all existing violations, except for `raftlogger` which
will need some extra TLC by the owning team (see cockroachdb#132262)

Closes cockroachdb#132258

Epic: none
Release note: None
@rickystewart rickystewart mentioned this issue Oct 9, 2024
Merged
rickystewart added a commit to rickystewart/cockroach that referenced this issue Oct 9, 2024
@rickystewart
lint: fix TestForbiddenImports
f17b46f 
We need to set `Dir` in the `packages.Load()` call or else the whole
thing doesn't work. It would be really nice if it threw an error instead
of simply doing nothing, but it is what it is. To guard against this in
the future I added an error if the list is empty.

Fix the test then all existing violations, except for `raftlogger` which
will need some extra TLC by the owning team (see cockroachdb#132262)

Closes cockroachdb#132258

Epic: none
Release note: None
rickystewart added a commit to rickystewart/cockroach that referenced this issue Oct 9, 2024
@rickystewart
lint: fix TestForbiddenImports
33b4e05 
We need to set `Dir` in the `packages.Load()` call or else the whole
thing doesn't work. It would be really nice if it threw an error instead
of simply doing nothing, but it is what it is. To guard against this in
the future I added an error if the list is empty.

Fix the test then all existing violations, except for `raftlogger` which
will need some extra TLC by the owning team (see cockroachdb#132262)

Closes cockroachdb#132258

Epic: none
Release note: None
rickystewart added a commit to rickystewart/cockroach that referenced this issue Oct 9, 2024
@rickystewart
lint: fix TestForbiddenImports
f6af2e6 
We need to set `Dir` in the `packages.Load()` call or else the whole
thing doesn't work. It would be really nice if it threw an error instead
of simply doing nothing, but it is what it is. To guard against this in
the future I added an error if the list is empty.

Fix the test then all existing violations, except for `raftlogger` which
will need some extra TLC by the owning team (see cockroachdb#132262)

Closes cockroachdb#132258

Epic: none
Release note: None
rickystewart added a commit to rickystewart/cockroach that referenced this issue Oct 9, 2024
@rickystewart
lint: fix TestForbiddenImports
3e8ea2c 
We need to set `Dir` in the `packages.Load()` call or else the whole
thing doesn't work. It would be really nice if it threw an error instead
of simply doing nothing, but it is what it is. To guard against this in
the future I added an error if the list is empty.

Fix the test then all existing violations, except for `raftlogger` which
will need some extra TLC by the owning team (see cockroachdb#132262)

Closes cockroachdb#132258

Epic: none
Release note: None
rickystewart added a commit to rickystewart/cockroach that referenced this issue Oct 9, 2024
@rickystewart
lint: fix TestForbiddenImports
f86871d 
We need to set `Dir` in the `packages.Load()` call or else the whole
thing doesn't work. It would be really nice if it threw an error instead
of simply doing nothing, but it is what it is. To guard against this in
the future I added an error if the list is empty.

Fix the test then all existing violations, except for `raftlogger` which
will need some extra TLC by the owning team (see cockroachdb#132262)

Closes cockroachdb#132258

Epic: none
Release note: None
@rickystewart rickystewart changed the title raft: raftlogger should use pkg/util/log instead of stdlib log package raft: raftlogger and rafttest should use pkg/util/log instead of stdlib log package Oct 9, 2024
rickystewart added a commit to rickystewart/cockroach that referenced this issue Oct 9, 2024
@rickystewart
lint: fix TestForbiddenImports
53c4735 
We need to set `Dir` in the `packages.Load()` call or else the whole
thing doesn't work. It would be really nice if it threw an error instead
of simply doing nothing, but it is what it is. To guard against this in
the future I added an error if the list is empty.

Fix the test then all existing violations, except for `raftlogger` which
will need some extra TLC by the owning team (see cockroachdb#132262)

Closes cockroachdb#132258

Epic: none
Release note: None
rickystewart added a commit to rickystewart/cockroach that referenced this issue Oct 10, 2024
@rickystewart
lint: fix TestForbiddenImports
4e662c4 
We need to set `Dir` in the `packages.Load()` call or else the whole
thing doesn't work. It would be really nice if it threw an error instead
of simply doing nothing, but it is what it is. To guard against this in
the future I added an error if the list is empty.

Fix the test then all existing violations, except for `raftlogger` which
will need some extra TLC by the owning team (see cockroachdb#132262)

Closes cockroachdb#132258

Epic: none
Release note: None
craig bot pushed a commit that referenced this issue Oct 10, 2024
@rickystewart @msbutler
Merge #132263 #132341
a69a2f3 
132263: lint: fix `TestForbiddenImports` r=rail,herkolategan a=rickystewart

We need to set `Dir` in the `packages.Load()` call or else the whole thing doesn't work. It would be really nice if it threw an error instead of simply doing nothing, but it is what it is. To guard against this in the future I added an error if the list is empty.

Fix the test then all existing violations, except for `raftlogger` and `rafttest` which will need some extra TLC by the owning team (see #132262)

Closes #132258

Epic: none
Release note: None

132341: roachtest: run perf tests with sql and kv mode r=jeffswenson a=msbutler

Epic: none

Release note: none

Co-authored-by: Ricky Stewart <[email protected]>
Co-authored-by: Michael Butler <[email protected]>
rickystewart added a commit to rickystewart/cockroach that referenced this issue Oct 10, 2024
@rickystewart
lint: fix TestForbiddenImports
46655b8 
We need to set `Dir` in the `packages.Load()` call or else the whole
thing doesn't work. It would be really nice if it threw an error instead
of simply doing nothing, but it is what it is. To guard against this in
the future I added an error if the list is empty.

Fix the test then all existing violations, except for `raftlogger` which
will need some extra TLC by the owning team (see cockroachdb#132262)

Closes cockroachdb#132258

Epic: none
Release note: None
rickystewart added a commit to rickystewart/cockroach that referenced this issue Oct 10, 2024
@rickystewart
lint: fix TestForbiddenImports
cf48f12 
We need to set `Dir` in the `packages.Load()` call or else the whole
thing doesn't work. It would be really nice if it threw an error instead
of simply doing nothing, but it is what it is. To guard against this in
the future I added an error if the list is empty.

Fix the test then all existing violations, except for `raftlogger` which
will need some extra TLC by the owning team (see cockroachdb#132262)

Closes cockroachdb#132258

Epic: none
Release note: None
This was referenced Oct 10, 2024
Merged
Merged
rickystewart added a commit to rickystewart/cockroach that referenced this issue Oct 10, 2024
@rickystewart
lint: fix TestForbiddenImports
63a0968 
We need to set `Dir` in the `packages.Load()` call or else the whole
thing doesn't work. It would be really nice if it threw an error instead
of simply doing nothing, but it is what it is. To guard against this in
the future I added an error if the list is empty.

Fix the test then all existing violations, except for `raftlogger` which
will need some extra TLC by the owning team (see cockroachdb#132262)

Closes cockroachdb#132258

Epic: none
Release note: None
This was referenced Oct 10, 2024
Merged
Merged
rickystewart added a commit to rickystewart/cockroach that referenced this issue Oct 10, 2024
@rickystewart
lint: fix TestForbiddenImports
fc3dccf 
We need to set `Dir` in the `packages.Load()` call or else the whole
thing doesn't work. It would be really nice if it threw an error instead
of simply doing nothing, but it is what it is. To guard against this in
the future I added an error if the list is empty.

Fix the test then all existing violations, except for `raftlogger` which
will need some extra TLC by the owning team (see cockroachdb#132262)

Closes cockroachdb#132258

Epic: none
Release note: None
rickystewart added a commit to rickystewart/cockroach that referenced this issue Oct 10, 2024
@rickystewart
lint: fix TestForbiddenImports
7d94e22 
We need to set `Dir` in the `packages.Load()` call or else the whole
thing doesn't work. It would be really nice if it threw an error instead
of simply doing nothing, but it is what it is. To guard against this in
the future I added an error if the list is empty.

Fix the test then all existing violations, except for `raftlogger` which
will need some extra TLC by the owning team (see cockroachdb#132262)

Closes cockroachdb#132258

Epic: none
Release note: None
mw5h pushed a commit to mw5h/cockroach that referenced this issue Oct 11, 2024
@rickystewart @mw5h
lint: fix TestForbiddenImports
62c4f1d 
We need to set `Dir` in the `packages.Load()` call or else the whole
thing doesn't work. It would be really nice if it threw an error instead
of simply doing nothing, but it is what it is. To guard against this in
the future I added an error if the list is empty.

Fix the test then all existing violations, except for `raftlogger` which
will need some extra TLC by the owning team (see cockroachdb#132262)

Closes cockroachdb#132258

Epic: none
Release note: None
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-kv KV Team
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@rickystewart