ensure hashmap init clears maps

[tjungblu]

This reorders some statements in the hashmap initialization to ensure we always start fresh, even when no pageids were passed to it.

fixes #791

--

Might be better to fix this before #775? would make an easier backport.

[ahrtr]

Suggested change

	p := (*common.Page)(unsafe.Pointer(&buf[0]))
	p := common.LoadPage(buf)

[tjungblu]

applied

[ahrtr]

Suggested change

	var buf [4096]byte
	buf := make([]byte, 4096)

[tjungblu]

applied

[ahrtr]

The test is a little misleading.

• If you reload (line 195) freelist, then you should also init/readIDs the freelist (f2) in the first place.
• In real production use case, the only case to call reload if when we need to rollback a writing TXN. But in this case, you do not rollback the freelist before calling reload.

[tjungblu]

Added your first suggestion. As for the second, we should hide the reload behind a Rollback as another refactoring step for #789

As of today, nothing stops you from reloading without the rollback.

[tjungblu]

Happy to hear other suggestions on how to replicate that inconsistent behavior though, feel free to send an alternative unit test.

[ahrtr]

As of today, nothing stops you from reloading without the rollback.

It means the interface isn't well designed. We need to continue to refactor the interface from users perspective. Will think about it.

[tjungblu]

I've rebased it against the refactoring now, let me know whether this is worth fixing or not. Otherwise we can close.

[ahrtr]

When you reload the freelist from a page, it means you already read the freelist from the page in the first place.

[ahrtr]

Please see the proposed unit test cases. We can revisit them when we update the interface later.

func TestFreeList_init(t *testing.T) {
	buf := make([]byte, 4096)
	f := newTestFreelist()
	f.Init(common.Pgids{5, 6, 8})

	p := common.LoadPage(buf)
	f.Write(p)

	f2 := newTestFreelist()
	f2.Read(p)
	require.Equal(t, common.Pgids{5, 6, 8}, f2.freePageIds())

	// When initializing the freelist with an empty list of page ID,
	// it should reset the freelist page IDs.
	f2.Init([]common.Pgid{})
	require.Equal(t, common.Pgids{}, f2.freePageIds())
}

func TestFreeList_reload(t *testing.T) {
	buf := make([]byte, 4096)
	f := newTestFreelist()
	f.Init(common.Pgids{5, 6, 8})

	p := common.LoadPage(buf)
	f.Write(p)

	f2 := newTestFreelist()
	f2.Read(p)
	require.Equal(t, common.Pgids{5, 6, 8}, f2.freePageIds())

	f2.Free(common.Txid(5), common.NewPage(10, common.LeafPageFlag, 0, 2))

	// reload shouldn't affect the pending list
	f2.Reload(p)

	require.Equal(t, common.Pgids{5, 6, 8}, f2.freePageIds())
	require.Equal(t, []common.Pgid{10, 11, 12}, f2.pendingPageIds()[5].ids)
}

[tjungblu]

Works for me, there's still some nil/empty inconsistency:

       	Error:      	Not equal: 
        	            	expected: common.Pgids{}
        	            	actual  : common.Pgids(nil)
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1,3 +1,2 @@
        	            	-(common.Pgids) {
        	            	-}
        	            	+(common.Pgids) <nil>

similar to here, I had to fix it with:
https://github.com/etcd-io/bbolt/pull/786/files#diff-1661bc3663fd3a2f6e7ebc479e8084bd0b1553a456792bdb0d005dbabee969f6R120

[tjungblu]

okay that required a bit more empty/nil changes unfortunately. I'll leave those as a separated commit now....

[ahrtr]

LGTM

cc @fuweid

[k8s-ci-robot]

[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