From 9824a04c1566fb34f0bb5e5e4082063331413c4b Mon Sep 17 00:00:00 2001 From: Soren Yang Date: Fri, 15 Dec 2023 15:47:32 +0800 Subject: [PATCH] chore(*): enable more revive.cyclomatic rules Signed-off-by: Soren Yang --- .golangci.yaml | 15 +++++--- Makefile | 4 +- common/batch/batcher.go | 2 +- common/batch/batcher_test.go | 3 -- server/kv/kv_pebble.go | 63 +------------------------------ server/kv/kv_pebble_test.go | 40 -------------------- server/kv/pebble_logger.go | 45 ++++++++++++++++++++++ server/kv/utils.go | 51 +++++++++++++++++++++++++ server/kv/utils_benchmark_test.go | 35 +++++++++++++++++ server/kv/utils_test.go | 45 ++++++++++++++++++++++ server/wal/wal_impl.go | 2 +- 11 files changed, 191 insertions(+), 114 deletions(-) create mode 100644 server/kv/pebble_logger.go create mode 100644 server/kv/utils.go create mode 100644 server/kv/utils_benchmark_test.go create mode 100644 server/kv/utils_test.go diff --git a/.golangci.yaml b/.golangci.yaml index 46067aff..d9213941 100644 --- a/.golangci.yaml +++ b/.golangci.yaml @@ -95,9 +95,9 @@ linters-settings: - 8 - name: cyclomatic severity: warning - disabled: true + disabled: false arguments: - - 3 + - 15 - name: function-length severity: warning disabled: true @@ -128,8 +128,11 @@ linters-settings: issues: fix: true - max-same-issues: 0 - max-issues-per-linter: 0 + max-same-issues: 3 + max-issues-per-linter: 50 + # include: + # - "EXC0012" + # - "EXC0014" exclude-rules: - path: _test\.go linters: @@ -151,7 +154,7 @@ issues: - path: _test\.go linters: - revive - text: "add-constant|cognitive-complexity|unused-parameter|unused-receiver|function-length|line-length-limit|unhandled-error|deep-exit|unchecked-type-assertion" + text: "add-constant|cognitive-complexity|unused-parameter|unused-receiver|function-length|line-length-limit|unhandled-error|deep-exit|unchecked-type-assertion|cyclomatic" - path: \.go linters: - revive @@ -159,7 +162,7 @@ issues: - path: maelstrom/ linters: - revive - text: "unchecked-type-assertion|unhandled-error|unused-receiver|unused-parameter|add-constant" + text: "unchecked-type-assertion|unhandled-error|unused-receiver|unused-parameter|add-constant|cyclomatic" - path: _grpc\.go linters: - revive diff --git a/Makefile b/Makefile index 36491f40..eb3c81fa 100644 --- a/Makefile +++ b/Makefile @@ -89,9 +89,9 @@ tla: OxiaReplication.tla license-check: - # go install github.com/palantir/go-license@latest + @command -v go-license > /dev/null || go install github.com/palantir/go-license@latest find . -type f -name '*.go' | grep -v '.pb.go' | xargs go-license --config=.github/license.yml --verify license-format: - # go install github.com/palantir/go-license@latest + @command -v go-license > /dev/null || go install github.com/palantir/go-license@latest find . -type f -name '*.go' | grep -v '.pb.go' | xargs go-license --config=.github/license.yml diff --git a/common/batch/batcher.go b/common/batch/batcher.go index bfa22a13..edebfc3c 100644 --- a/common/batch/batcher.go +++ b/common/batch/batcher.go @@ -58,7 +58,7 @@ func (b *batcherImpl) failCall(call any, err error) { batch.Fail(err) } -func (b *batcherImpl) Run() { +func (b *batcherImpl) Run() { //nolint:revive var batch Batch var timer *time.Timer var timeout <-chan time.Time diff --git a/common/batch/batcher_test.go b/common/batch/batcher_test.go index d8d064f9..dbc03204 100644 --- a/common/batch/batcher_test.go +++ b/common/batch/batcher_test.go @@ -79,9 +79,6 @@ func TestBatcher(t *testing.T) { MaxRequestsPerBatch: item.maxSize, } batcher := factory.NewBatcher(batchFactory) - - go batcher.Run() - batcher.Add(1) if item.closeImmediately { diff --git a/server/kv/kv_pebble.go b/server/kv/kv_pebble.go index e3ed5ce0..7e7e033c 100644 --- a/server/kv/kv_pebble.go +++ b/server/kv/kv_pebble.go @@ -15,7 +15,6 @@ package kv import ( - "bytes" "fmt" "io" "log/slog" @@ -35,7 +34,7 @@ import ( var ( OxiaSlashSpanComparer = &pebble.Comparer{ - Compare: CompareWithSlash, + Compare: compareWithSlash, Equal: pebble.DefaultComparer.Equal, AbbreviatedKey: pebble.DefaultComparer.AbbreviatedKey, FormatKey: pebble.DefaultComparer.FormatKey, @@ -206,7 +205,7 @@ func newKVPebble(factory *PebbleFactory, namespace string, shardId int64) (KV, e }, FS: vfs.Default, DisableWAL: true, - Logger: &PebbleLogger{ + Logger: &pebbleLogger{ slog.With( slog.String("component", "pebble"), slog.Int64("shard", shardId), @@ -549,64 +548,6 @@ func (p *PebbleReverseIterator) Value() ([]byte, error) { return res, err } -// Pebble logger wrapper - -type PebbleLogger struct { - zl *slog.Logger -} - -func (pl *PebbleLogger) Infof(format string, args ...any) { - pl.zl.Info( - fmt.Sprintf(format, args...), - ) -} - -func (pl *PebbleLogger) Errorf(format string, args ...any) { - pl.zl.Error( - fmt.Sprintf(format, args...), - ) -} - -func (pl *PebbleLogger) Fatalf(format string, args ...any) { - pl.zl.Warn( - fmt.Sprintf(format, args...), - ) - os.Exit(1) -} - -func CompareWithSlash(a, b []byte) int { - for len(a) > 0 && len(b) > 0 { - idxA, idxB := bytes.IndexByte(a, '/'), bytes.IndexByte(b, '/') - switch { - case idxA < 0 && idxB < 0: - return bytes.Compare(a, b) - case idxA < 0 && idxB >= 0: - return -1 - case idxA >= 0 && idxB < 0: - return +1 - } - - // At this point, both slices have '/' - spanA, spanB := a[:idxA], b[:idxB] - - spanRes := bytes.Compare(spanA, spanB) - if spanRes != 0 { - return spanRes - } - - a, b = a[idxA+1:], b[idxB+1:] - } - - switch { - case len(a) < len(b): - return -1 - case len(a) > len(b): - return +1 - } - - return 0 -} - type pebbleSnapshot struct { path string files []string diff --git a/server/kv/kv_pebble_test.go b/server/kv/kv_pebble_test.go index 85d38f39..3857666e 100644 --- a/server/kv/kv_pebble_test.go +++ b/server/kv/kv_pebble_test.go @@ -15,7 +15,6 @@ package kv import ( - "bytes" "fmt" "os" "path/filepath" @@ -220,45 +219,6 @@ func TestPebbleRangeScan(t *testing.T) { assert.NoError(t, factory.Close()) } -var benchKeyA = []byte("/test/aaaaaaaaaaa/bbbbbbbbbbb/cccccccccccc/dddddddddddddd") -var benchKeyB = []byte("/test/aaaaaaaaaaa/bbbbbbbbbbb/ccccccccccccddddddddddddddd") - -func BenchmarkStandardCompare(b *testing.B) { - for i := 0; i < b.N; i++ { - bytes.Compare(benchKeyA, benchKeyB) - } -} - -func BenchmarkCompareWithSlash(b *testing.B) { - for i := 0; i < b.N; i++ { - CompareWithSlash(benchKeyA, benchKeyB) - } -} - -func TestCompareWithSlash(t *testing.T) { - assert.Equal(t, 0, CompareWithSlash([]byte("aaaaa"), []byte("aaaaa"))) - assert.Equal(t, -1, CompareWithSlash([]byte("aaaaa"), []byte("zzzzz"))) - assert.Equal(t, +1, CompareWithSlash([]byte("bbbbb"), []byte("aaaaa"))) - - assert.Equal(t, +1, CompareWithSlash([]byte("aaaaa"), []byte(""))) - assert.Equal(t, -1, CompareWithSlash([]byte(""), []byte("aaaaaa"))) - assert.Equal(t, 0, CompareWithSlash([]byte(""), []byte(""))) - - assert.Equal(t, -1, CompareWithSlash([]byte("aaaaa"), []byte("aaaaaaaaaaa"))) - assert.Equal(t, +1, CompareWithSlash([]byte("aaaaaaaaaaa"), []byte("aaa"))) - - assert.Equal(t, -1, CompareWithSlash([]byte("a"), []byte("/"))) - assert.Equal(t, +1, CompareWithSlash([]byte("/"), []byte("a"))) - - assert.Equal(t, -1, CompareWithSlash([]byte("/aaaa"), []byte("/bbbbb"))) - assert.Equal(t, -1, CompareWithSlash([]byte("/aaaa"), []byte("/aa/a"))) - assert.Equal(t, -1, CompareWithSlash([]byte("/aaaa/a"), []byte("/aaaa/b"))) - assert.Equal(t, +1, CompareWithSlash([]byte("/aaaa/a/a"), []byte("/bbbbbbbbbb"))) - assert.Equal(t, +1, CompareWithSlash([]byte("/aaaa/a/a"), []byte("/aaaa/bbbbbbbbbb"))) - - assert.Equal(t, +1, CompareWithSlash([]byte("/a/b/a/a/a"), []byte("/a/b/a/b"))) -} - func TestPebbleRangeScanWithSlashOrder(t *testing.T) { keys := []string{ "/a/a/a/zzzzzz", diff --git a/server/kv/pebble_logger.go b/server/kv/pebble_logger.go new file mode 100644 index 00000000..6402f391 --- /dev/null +++ b/server/kv/pebble_logger.go @@ -0,0 +1,45 @@ +// Copyright 2023 StreamNative, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package kv + +import ( + "fmt" + "log/slog" + "os" +) + +// pebbleLogger is the wrapper of slog to implement pebbel's logger interface. +type pebbleLogger struct { + zl *slog.Logger +} + +func (pl *pebbleLogger) Infof(format string, args ...any) { + pl.zl.Info( + fmt.Sprintf(format, args...), + ) +} + +func (pl *pebbleLogger) Errorf(format string, args ...any) { + pl.zl.Error( + fmt.Sprintf(format, args...), + ) +} + +func (pl *pebbleLogger) Fatalf(format string, args ...any) { + pl.zl.Warn( + fmt.Sprintf(format, args...), + ) + os.Exit(1) +} diff --git a/server/kv/utils.go b/server/kv/utils.go new file mode 100644 index 00000000..af564241 --- /dev/null +++ b/server/kv/utils.go @@ -0,0 +1,51 @@ +// Copyright 2023 StreamNative, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package kv + +import ( + "bytes" +) + +func compareWithSlash(a, b []byte) int { + for len(a) > 0 && len(b) > 0 { + idxA, idxB := bytes.IndexByte(a, '/'), bytes.IndexByte(b, '/') + switch { + case idxA < 0 && idxB < 0: + return bytes.Compare(a, b) + case idxA < 0 && idxB >= 0: + return -1 + case idxA >= 0 && idxB < 0: + return +1 + } + + // At this point, both slices have '/' + spanA, spanB := a[:idxA], b[:idxB] + spanRes := bytes.Compare(spanA, spanB) + if spanRes != 0 { + return spanRes + } + + a, b = a[idxA+1:], b[idxB+1:] + } + + switch { + case len(a) < len(b): + return -1 + case len(a) > len(b): + return +1 + } + + return 0 +} diff --git a/server/kv/utils_benchmark_test.go b/server/kv/utils_benchmark_test.go new file mode 100644 index 00000000..2fa31135 --- /dev/null +++ b/server/kv/utils_benchmark_test.go @@ -0,0 +1,35 @@ +// Copyright 2023 StreamNative, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package kv + +import ( + "bytes" + "testing" +) + +var benchKeyA = []byte("/test/aaaaaaaaaaa/bbbbbbbbbbb/cccccccccccc/dddddddddddddd") +var benchKeyB = []byte("/test/aaaaaaaaaaa/bbbbbbbbbbb/ccccccccccccddddddddddddddd") + +func Benchmark_StandardCompare(b *testing.B) { + for i := 0; i < b.N; i++ { + bytes.Compare(benchKeyA, benchKeyB) + } +} + +func Benchmark_CompareWithSlash(b *testing.B) { + for i := 0; i < b.N; i++ { + compareWithSlash(benchKeyA, benchKeyB) + } +} diff --git a/server/kv/utils_test.go b/server/kv/utils_test.go new file mode 100644 index 00000000..1386e69d --- /dev/null +++ b/server/kv/utils_test.go @@ -0,0 +1,45 @@ +// Copyright 2023 StreamNative, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package kv + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestCompareWithSlash(t *testing.T) { + assert.Equal(t, 0, compareWithSlash([]byte("aaaaa"), []byte("aaaaa"))) + assert.Equal(t, -1, compareWithSlash([]byte("aaaaa"), []byte("zzzzz"))) + assert.Equal(t, +1, compareWithSlash([]byte("bbbbb"), []byte("aaaaa"))) + + assert.Equal(t, +1, compareWithSlash([]byte("aaaaa"), []byte(""))) + assert.Equal(t, -1, compareWithSlash([]byte(""), []byte("aaaaaa"))) + assert.Equal(t, 0, compareWithSlash([]byte(""), []byte(""))) + + assert.Equal(t, -1, compareWithSlash([]byte("aaaaa"), []byte("aaaaaaaaaaa"))) + assert.Equal(t, +1, compareWithSlash([]byte("aaaaaaaaaaa"), []byte("aaa"))) + + assert.Equal(t, -1, compareWithSlash([]byte("a"), []byte("/"))) + assert.Equal(t, +1, compareWithSlash([]byte("/"), []byte("a"))) + + assert.Equal(t, -1, compareWithSlash([]byte("/aaaa"), []byte("/bbbbb"))) + assert.Equal(t, -1, compareWithSlash([]byte("/aaaa"), []byte("/aa/a"))) + assert.Equal(t, -1, compareWithSlash([]byte("/aaaa/a"), []byte("/aaaa/b"))) + assert.Equal(t, +1, compareWithSlash([]byte("/aaaa/a/a"), []byte("/bbbbbbbbbb"))) + assert.Equal(t, +1, compareWithSlash([]byte("/aaaa/a/a"), []byte("/aaaa/bbbbbbbbbb"))) + + assert.Equal(t, +1, compareWithSlash([]byte("/a/b/a/a/a"), []byte("/a/b/a/b"))) +} diff --git a/server/wal/wal_impl.go b/server/wal/wal_impl.go index e2d6685e..9e8ee5a4 100644 --- a/server/wal/wal_impl.go +++ b/server/wal/wal_impl.go @@ -430,7 +430,7 @@ func (t *wal) Delete() error { ) } -func (t *wal) TruncateLog(lastSafeOffset int64) (int64, error) { +func (t *wal) TruncateLog(lastSafeOffset int64) (int64, error) { //nolint:revive if lastSafeOffset == InvalidOffset { if err := t.Clear(); err != nil { return InvalidOffset, err