add freelist interface unit tests #786

tjungblu · 2024-07-04T15:46:59Z

No description provided.

tjungblu · 2024-07-22T08:16:30Z

still pending the work in #788, #796, #794 and #792

ahrtr · 2024-08-02T13:51:37Z

Please put the changes for freelist.go amd shared.go into a separate PR, which should be able to be merged soon. thx

Reload and NoSyncReload have duplicated code, this unifies both for later refactoring. This PR is split from etcd-io#786, where the tests found differences on reloading and nil/empty initializations. Added some more clarifications in godocs for certain panic behavior and expected returns on the interface. Signed-off-by: Thomas Jungblut <[email protected]>

ahrtr · 2024-08-07T15:13:52Z

Thanks @tjungblu . Please signoff the commit, I will take a look later.

tjungblu · 2024-08-07T15:19:30Z

done, sorry some debug commit got in between

ahrtr · 2024-08-07T15:30:35Z

internal/freelist/array_test.go

+
+func TestInvalidArrayAllocation(t *testing.T) {
+	f := NewArrayFreelist()
+	ids := []common.Pgid{1}


Suggested change

ids := []common.Pgid{1}

// page 0 and 1 are reserved for meta pages, so they should never be free pages.

ids := []common.Pgid{1}

ahrtr · 2024-08-07T15:42:26Z

internal/freelist/freelist_test.go

+	require.Empty(t, f.pendingPageIds())
+	require.False(t, f.Freed(12))
+
+	// we still hold an allotx reference to page 12 through txid 99 - let's try to free it again


why txid 99? :) page 12 might be allocated by any txid which is < 100.

Suggested change

// we still hold an allotx reference to page 12 through txid 99 - let's try to free it again

// Let's try to free it again

I've removed this comment. txid 99 was because we had this little hack:
d72e6bf#diff-d722be595713f8fc49d086ab1968ff45f223c2b1f99bc0f7ae65fbdde3132c40L70-L73

ahrtr · 2024-08-07T16:05:44Z

internal/freelist/freelist_test.go

+	freelist.Free(common.Txid(10), common.NewPage(10, common.LeafPageFlag, 0, 2))
+	freelist.Free(common.Txid(11), common.NewPage(20, common.LeafPageFlag, 0, 2))
+	freelist.Free(common.Txid(12), common.NewPage(30, common.LeafPageFlag, 0, 2))


This doesn't make sense. Only one RW TXN is allowed at a time. The three Free calls are for three different TXID, it means there are 3 concurrent RW TXNs.

it's a simple test setup, we can sprinkle the free calls around the AddReadOnlyTXID if this is more sensible and readable for you.

I think we need to satisfy the following conditions,

Only one RW TXN is allowed at a time. So yon can't call Free with a higher txid before you commit the previous RW TXN.

When you create a readonly TXN, its TXID should equal to the latest committed RW TXN's txid.

shall we add an assertion for this in a separate PR?

shall we add an assertion for this in a separate PR?

It's hard to add such assertion/verification in freelist layer, because the semantics is guaranteed by upper layer (bbolt) instead of freelist. It's also an indication that the freelist API isn't well designed.

Can you move this test into a separate PR so that we can merge this PR? thx

yep, will do. brb

ahrtr · 2024-08-07T16:11:51Z

internal/freelist/freelist_test.go

+	freelist.Free(common.Txid(5), common.NewPage(5, common.LeafPageFlag, 0, 4))
+	freelist.Reload(p)
+	requirePages(t, freelist, common.Pgids{}, common.Pgids{5, 6, 7, 8, 9})


We can keep it as it's for now. But actually reload is only used in rollback case, so there should be a rollback operation. We can revisit this when we refactor the interface.

ahrtr · 2024-08-20T08:37:48Z

@tjungblu please rebase this PR. thx

adding more unit tests for better coverage of the interface. Signed-off-by: Thomas Jungblut <[email protected]>

tjungblu · 2024-08-20T08:45:15Z

done, thanks @ahrtr

ahrtr

LGTM

Thanks @tjungblu

k8s-ci-robot · 2024-08-20T09:36:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahrtr, tjungblu

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [ahrtr]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Reload and NoSyncReload have duplicated code, this unifies both for later refactoring. This PR is split from etcd-io#786, where the tests found differences on reloading and nil/empty initializations. Added some more clarifications in godocs for certain panic behavior and expected returns on the interface. Signed-off-by: Thomas Jungblut <[email protected]> Signed-off-by: samuelbartels20 <[email protected]>

k8s-ci-robot added do-not-merge/work-in-progress size/XXL labels Jul 4, 2024

tjungblu force-pushed the fl_iface_tests branch 7 times, most recently from a7742ab to e00d49d Compare July 9, 2024 09:32

tjungblu mentioned this pull request Jul 9, 2024

freelist hashmap/array inconsistency on page reload #791

Closed

tjungblu force-pushed the fl_iface_tests branch from e00d49d to 87e560e Compare July 22, 2024 08:15

k8s-ci-robot added size/L and removed size/XXL labels Jul 22, 2024

tjungblu changed the title ~~[WIP] more freelist interface tests~~ add freelist interface unit tests Jul 22, 2024

tjungblu marked this pull request as ready for review July 22, 2024 08:15

k8s-ci-robot removed the do-not-merge/work-in-progress label Jul 22, 2024

tjungblu force-pushed the fl_iface_tests branch 3 times, most recently from 23d74f2 to 4a12741 Compare July 23, 2024 08:53

tjungblu force-pushed the fl_iface_tests branch 4 times, most recently from 815322f to c4a4b8e Compare July 31, 2024 15:51

tjungblu mentioned this pull request Aug 5, 2024

Dedupe Reload/NoSyncReload, prefer empty instead of nil init #804

Merged

tjungblu force-pushed the fl_iface_tests branch 2 times, most recently from 629ac9c to e53f4c4 Compare August 6, 2024 16:08

tjungblu force-pushed the fl_iface_tests branch from e53f4c4 to 9dc0684 Compare August 6, 2024 16:28

tjungblu force-pushed the fl_iface_tests branch from 9dc0684 to 24b3ed5 Compare August 7, 2024 15:19

ahrtr reviewed Aug 7, 2024

View reviewed changes

tjungblu force-pushed the fl_iface_tests branch from 24b3ed5 to e457dac Compare August 8, 2024 12:42

tjungblu force-pushed the fl_iface_tests branch from e457dac to ca560a1 Compare August 20, 2024 08:18

tjungblu force-pushed the fl_iface_tests branch from ca560a1 to 7348ab8 Compare August 20, 2024 08:42

add freelist interface unit tests

3ce0fd0

adding more unit tests for better coverage of the interface. Signed-off-by: Thomas Jungblut <[email protected]>

tjungblu force-pushed the fl_iface_tests branch from 7348ab8 to 3ce0fd0 Compare August 20, 2024 08:44

ahrtr approved these changes Aug 20, 2024

View reviewed changes

k8s-ci-robot added the approved label Aug 20, 2024

ahrtr merged commit c74748e into etcd-io:main Aug 20, 2024
18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add freelist interface unit tests #786

add freelist interface unit tests #786

tjungblu commented Jul 4, 2024

tjungblu commented Jul 22, 2024

ahrtr commented Aug 2, 2024

ahrtr commented Aug 7, 2024

tjungblu commented Aug 7, 2024

ahrtr Aug 7, 2024

tjungblu Aug 8, 2024

ahrtr Aug 7, 2024

tjungblu Aug 8, 2024

ahrtr Aug 7, 2024

tjungblu Aug 8, 2024

ahrtr Aug 8, 2024

tjungblu Aug 9, 2024

ahrtr Aug 12, 2024

ahrtr Aug 19, 2024

tjungblu Aug 20, 2024

tjungblu Aug 20, 2024

ahrtr Aug 7, 2024

tjungblu Aug 8, 2024

ahrtr commented Aug 20, 2024

tjungblu commented Aug 20, 2024

ahrtr left a comment

k8s-ci-robot commented Aug 20, 2024

	ids := []common.Pgid{1}
	// page 0 and 1 are reserved for meta pages, so they should never be free pages.
	ids := []common.Pgid{1}

	// we still hold an allotx reference to page 12 through txid 99 - let's try to free it again
	// Let's try to free it again

add freelist interface unit tests #786

add freelist interface unit tests #786

Conversation

tjungblu commented Jul 4, 2024

tjungblu commented Jul 22, 2024

ahrtr commented Aug 2, 2024

ahrtr commented Aug 7, 2024

tjungblu commented Aug 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahrtr commented Aug 20, 2024

tjungblu commented Aug 20, 2024

ahrtr left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Aug 20, 2024