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

add freelist interface unit tests #786

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

add freelist interface unit tests #786

wants to merge 1 commit into from

Conversation

tjungblu
Copy link
Contributor

@tjungblu tjungblu commented Jul 4, 2024

No description provided.

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: tjungblu
Once this PR has been reviewed and has the lgtm label, please assign ahrtr for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

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

@tjungblu tjungblu force-pushed the fl_iface_tests branch 7 times, most recently from a7742ab to e00d49d Compare July 9, 2024 09:32
@tjungblu tjungblu mentioned this pull request Jul 9, 2024
Closed
@tjungblu tjungblu changed the title [WIP] more freelist interface tests add freelist interface unit tests Jul 22, 2024
@tjungblu tjungblu marked this pull request as ready for review July 22, 2024 08:15
@tjungblu
Copy link
Contributor Author

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

@tjungblu tjungblu force-pushed the fl_iface_tests branch 3 times, most recently from 23d74f2 to 4a12741 Compare July 23, 2024 08:53
@tjungblu tjungblu force-pushed the fl_iface_tests branch 4 times, most recently from 815322f to c4a4b8e Compare July 31, 2024 15:51
@ahrtr
Copy link
Member

ahrtr commented Aug 2, 2024

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

tjungblu added a commit to tjungblu/bbolt that referenced this pull request Aug 5, 2024
@tjungblu
Dedupe Reload/NoSyncReload, prefer empty instead of nil init
4061583 
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]>
@tjungblu tjungblu mentioned this pull request Aug 5, 2024
Merged
tjungblu added a commit to tjungblu/bbolt that referenced this pull request Aug 6, 2024
@tjungblu
Dedupe Reload/NoSyncReload, prefer empty instead of nil init
33b71a5 
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]>
@tjungblu tjungblu force-pushed the fl_iface_tests branch 2 times, most recently from e53f4c4 to 9dc0684 Compare August 6, 2024 16:28
@ahrtr
Copy link
Member

ahrtr commented Aug 7, 2024

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

@tjungblu
Copy link
Contributor Author

tjungblu commented Aug 7, 2024

done, sorry some debug commit got in between

ahrtr
ahrtr reviewed Aug 7, 2024
View reviewed changes
internal/freelist/array_test.go

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

Choose a reason for hiding this comment

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

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}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

applied

internal/freelist/freelist_test.go Outdated
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
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

internal/freelist/freelist_test.go
Comment on lines +454 to +455
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))
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

internal/freelist/freelist_test.go
Comment on lines +516 to +517
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})
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack

@tjungblu
add freelist interface unit tests
e457dac 
adding more unit tests for better coverage of the interface and some
implementation details.

Signed-off-by: Thomas Jungblut <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahrtr ahrtr ahrtr left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
size/L
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@tjungblu @k8s-ci-robot @ahrtr