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

go.mod: revert comparer change and bump Pebble to 0f785fec58c0 #131366

Merged

Conversation

RaduBerinde
Copy link
Member

@RaduBerinde RaduBerinde commented Sep 25, 2024

The comparer changes effectively revert #128043, which can cause
replica inconsistency during/after upgrades.

Changes:

  • 0f785fec metamorphic: abridge failure output
  • be56747f db: fix overlap check for flushable ingest excises
  • 2569414a db: remove race in TestCrashOpenCrashAfterWALCreation
  • 575f7a04 sstable: support columnar blocks in Layout.Describe
  • c88c7471 github: fix code cover publish workflow
  • c0fa4a9c sstable: populate CompareSuffixes on test4bSuffixComparer
  • 3a76074f sstable: set IndexPartitions property in columnar sstable writer
  • 0595c1fb colblk: define behavior of KeyWriter.ComparePrev with no previous
  • 01dcf575 base: make comparer tolerate empty keys
  • d73ab80f db: allow excises to unconditionally be flushable ingests
  • b34a3937 base: allow CompareSuffixes to be stricter than Compare
  • 90356021 db: refactor replayWAL to use flushes to make versionEdits

Informs: #130533

Release note: none.
Epic: none.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@RaduBerinde RaduBerinde requested a review from a team as a code owner September 25, 2024 19:57
Copy link
Collaborator

@jbowens jbowens left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 8 of 8 files at r1, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @itsbilal)

@itsbilal
Copy link
Member

Might be worth holding off on this for at least a day so we can get the fix for cockroachdb/pebble#3963

@RaduBerinde
Copy link
Member Author

Yes, agreed.

@RaduBerinde RaduBerinde changed the title go.mod: revert comparer change and bump Pebble to 01dcf5752d56 go.mod: revert comparer change and bump Pebble to 0f785fec58c0 Sep 26, 2024
@RaduBerinde
Copy link
Member Author

Updated to the current Pebble master.

Copy link
Member

@itsbilal itsbilal left a comment

Choose a reason for hiding this comment

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

LGTM

@RaduBerinde
Copy link
Member Author

I'm getting this test error:

=== RUN   TestStoreMetrics
    test_log_scope.go:170: test logs captured to: outputs.zip/logTestStoreMetrics1690765082
    test_log_scope.go:81: use -show-logs to present logs inline
    client_metrics_test.go:389: counter rocksdb.block.cache.hits = 0 < min 10
    client_metrics_test.go:389: counter rocksdb.block.cache.hits = 0 < min 10
    client_metrics_test.go:393: -- test log scope end --

Any ideas why this changed?

@jbowens
Copy link
Collaborator

jbowens commented Sep 27, 2024

I don't like that test. I've run afoul of it before, because it ends up asserting on details of the storage engine mechanics. I've got no idea why we'd be getting zero block cache hits though.

@itsbilal
Copy link
Member

Looking at the logs, it's sometimes possible for the keys to just stick around in the WAL/memtable. Let me see if I can force a flush in the test.

@itsbilal
Copy link
Member

This should deflake this test:

deflake.patch

diff --git a/pkg/kv/kvserver/client_metrics_test.go b/pkg/kv/kvserver/client_metrics_test.go
index 232f9c9cfbf..eb6413c6ce9 100644
--- a/pkg/kv/kvserver/client_metrics_test.go
+++ b/pkg/kv/kvserver/client_metrics_test.go
@@ -262,7 +262,7 @@ func TestStoreMetrics(t *testing.T) {
                        },
                }
                stickyServerArgs[i] = base.TestServerArgs{
-                       CacheSize:  1 << 20, /* 1 MiB */
+                       CacheSize:  2 << 20, /* 2 MiB */
                        StoreSpecs: []base.StoreSpec{spec},
                        Knobs: base.TestingKnobs{
                                Server: &server.TestingKnobs{

@RaduBerinde
Copy link
Member Author

Thank you!! What's the explanation why a 1MB cache leads to no hits?

The comparer changes effectively revert cockroachdb#128043, which can cause
replica inconsistency during/after upgrades.

Changes:

 * [`0f785fec`](cockroachdb/pebble@0f785fec) metamorphic: abridge failure output
 * [`be56747f`](cockroachdb/pebble@be56747f) db: fix overlap check for flushable ingest excises
 * [`2569414a`](cockroachdb/pebble@2569414a) db: remove race in TestCrashOpenCrashAfterWALCreation
 * [`575f7a04`](cockroachdb/pebble@575f7a04) sstable: support columnar blocks in Layout.Describe
 * [`c88c7471`](cockroachdb/pebble@c88c7471) github: fix code cover publish workflow
 * [`c0fa4a9c`](cockroachdb/pebble@c0fa4a9c) sstable: populate CompareSuffixes on test4bSuffixComparer
 * [`3a76074f`](cockroachdb/pebble@3a76074f) sstable: set IndexPartitions property in columnar sstable writer
 * [`0595c1fb`](cockroachdb/pebble@0595c1fb) colblk: define behavior of KeyWriter.ComparePrev with no previous
 * [`01dcf575`](cockroachdb/pebble@01dcf575) base: make comparer tolerate empty keys
 * [`d73ab80f`](cockroachdb/pebble@d73ab80f) db: allow excises to unconditionally be flushable ingests
 * [`b34a3937`](cockroachdb/pebble@b34a3937) base: allow CompareSuffixes to be stricter than Compare
 * [`90356021`](cockroachdb/pebble@90356021) db: refactor replayWAL to use flushes to make versionEdits

Informs: cockroachdb#130533

Release note: none.
Epic: none.
@RaduBerinde
Copy link
Member Author

bors r+

@craig craig bot merged commit 630e676 into cockroachdb:master Sep 27, 2024
23 checks passed
@itsbilal
Copy link
Member

Thank you!! What's the explanation why a 1MB cache leads to no hits?

I actually don't know either, but I also wasn't able to really understand what the test was doing so....

@RaduBerinde RaduBerinde deleted the radu/pebble-master-01dcf5752d56 branch September 28, 2024 04:27
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.

4 participants