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

wal: implement failoverManager.{Obsolete,Stats} and log recycling #3328

Merged
merged 1 commit into from
Feb 22, 2024

Conversation

sumeerbhola
Copy link
Collaborator

Informs #3230

Informs CRDB-35401

@sumeerbhola sumeerbhola requested review from jbowens and a team February 21, 2024 19:57
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator Author

@sumeerbhola sumeerbhola left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 8 files reviewed, 1 unresolved discussion (waiting on @jbowens)


wal/failover_manager_test.go line 496 at r1 (raw file):

// TODO(sumeer): test failoverManager.{List,Obsolete,Stats}
// TODO(sumeer): test failoverWriter.getLog

The todos to test the new code. Existing tests pass.
If you need these changes merged to get unblocked, I can write the tests in a followup PR.

@sumeerbhola sumeerbhola force-pushed the failover_recycle branch 2 times, most recently from 37c4058 to 0b6f1a0 Compare February 22, 2024 02:57
Copy link
Collaborator Author

@sumeerbhola sumeerbhola left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 11 files reviewed, 1 unresolved discussion (waiting on @jbowens)


wal/failover_manager_test.go line 496 at r1 (raw file):

Previously, sumeerbhola wrote…

The todos to test the new code. Existing tests pass.
If you need these changes merged to get unblocked, I can write the tests in a followup PR.

tests are done

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_strong:

Reviewed 5 of 8 files at r1, 2 of 5 files at r2, 1 of 2 files at r3, all commit messages.
Reviewable status: 8 of 11 files reviewed, 2 unresolved discussions (waiting on @sumeerbhola)


wal/failover_manager.go line 602 at r3 (raw file):

	wm.mu.Lock()
	defer wm.mu.Unlock()
	wm.mu.closedWALs = append(wm.mu.closedWALs, wm.mu.ww.getLog())

wdyt of passing the logicalLogWithSizesEtc in to the writerClosed callback to make this control flow a little more explicit?

Copy link
Collaborator Author

@sumeerbhola sumeerbhola left a comment

Choose a reason for hiding this comment

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

TFTR!

Reviewable status: 5 of 11 files reviewed, 2 unresolved discussions (waiting on @jbowens)


wal/failover_manager.go line 602 at r3 (raw file):

Previously, jbowens (Jackson Owens) wrote…

wdyt of passing the logicalLogWithSizesEtc in to the writerClosed callback to make this control flow a little more explicit?

Done

@sumeerbhola
Copy link
Collaborator Author

following test failure is unrelated:

--- FAIL: TestGeneratorRandom (0.08s)
    --- FAIL: TestGeneratorRandom/config=default (0.08s)
panic: could not generate unique key [recovered]
	panic: could not generate unique key [recovered]
	panic: could not generate unique key

@sumeerbhola sumeerbhola merged commit 2537487 into cockroachdb:master Feb 22, 2024
10 of 11 checks passed
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.

3 participants