Tests for unsharding PR #99

schomatis · 2021-06-23T23:06:13Z

Tests for #94.

Concurrent test being worked independently in #106.

Preliminary design review requested:

Only needed for io/directory_test.go, particularly TestHAMTEnumerationWhenComputingSize and countGetsDS.(Tests failing for Ubuntu but that might be expected, see more details in TestHAMTEnumerationWhenComputingSize in linked file.)
Focus is on testing the sizeBelowThreshold logic and making sure we are not fetching more nodes than we should. Making this the priority as it's the trickiest part to test since it's an optimization and the unsharding will work just the same without it which makes problems here harder to detect (the rest is evaluating just if the directory switched from HAMT to Basic on different scenarios, see TestUpgradeableDirectory from base PR as an illustration).
Lots of FIXMES and rough edges around internal interfaces to simplify HAMT logic and make it easier to test. Addressing them now it's not a priority of this review but any feedback welcome. Priority was to get something working ASAP for an early review.

aschmahmann

Left some general comments on approach here since it looks like the PR is still a WIP.

aschmahmann · 2021-08-12T01:39:30Z

hamt/hamt.go

+	// FIXME: Remove. We don't actually store "value nodes". This confusing
+	//  abstraction just removes the maxpadlen from the link names to extract
+	//  the actual value link the trie is storing.


My understanding of your comment here is that val.Name and maxpadlen can be used to compute key and so we shouldn't bother caching it here and just recompute it when needed. Is that correct?

Also, can you clarify if this a FIXME intended for this set of PRs?

hamt/hamt.go

aschmahmann · 2021-08-25T10:02:24Z

hamt/hamt.go

+// EnumLinksAsync returns a channel which will receive Links in the directory
+// as they are enumerated, where order is not guaranteed
+func (ds *Shard) EnumAllAsync(ctx context.Context) <-chan format.LinkResult {


This isn't needed anymore given EnumLinksAsync should be concurrent, right?

Seems not, and also seems to be unused. Removing

aschmahmann · 2021-08-25T10:05:29Z

hamt/util.go

+//  the fake hash as in io.SetAndPrevious through `newHashBits()` and pass
+//  it as an argument making the hash independent of tree manipulation; that
+//  sounds as the correct way to go in general and we wouldn't need this.)
+func CreateCompleteHAMT(ds ipld.DAGService, treeHeight int) (ipld.Node, error) {


functions only used by go tests should live in _test.go files if possible.

Note: if you're running into dependency cycle issues or issues with breaking the API a tool that may be your friend here is the internal package which allows things to be exported for use within the module, but not outside of it.

Note: if you're running into dependency cycle issues or issues with breaking the API a tool that may be your friend here is the internal package which allows things to be exported for use within the module, but not outside of it.

Could you expand a bit more on that? I'm not hitting many useful results Googling golang internal package import cycle.

An example of using the internal package (although they probably abuse it) is in go-bitswap e.g. https://github.com/ipfs/go-bitswap/blob/master/internal/peermanager/peermanager.go

io/directory_test.go

* wip * fix CreateCompleteHAMT test * relax test constraint * fix value of nodes fetched in test according to a BFS walk

* move CreateCompleteHAMT to an internal package * change package name: internal -> private * move hash function to internal pacakge * make link size function internal * make shard width private * add internal * go fmt * fix import order * export DefaultShardWidth back again * move IdHash to private package * move LinkSizeFunction to private * add linksize

aschmahmann · 2021-10-21T20:28:04Z

hamt/hamt.go

+// FIXME: Check which functions do we need to actually expose.
+func (ds *Shard) EnumAll(ctx context.Context) ([]*ipld.Link, error) {
+	var links []*ipld.Link
+
+	linkResults := ds.EnumAllAsync(ctx)
+
+	for linkResult := range linkResults {
+		if linkResult.Err != nil {
+			return links, linkResult.Err
+		}
+		links = append(links, linkResult.Link)
+	}
+	return links, nil
+}
+


It looks like this function is a duplicate of EnumLinks

@schomatis I pushed a commit removing this

aschmahmann · 2021-10-21T20:32:37Z

hamt/hamt.go

+	internal.HAMTHashFunction = murmur3Hash
+}
+
+func (ds *Shard) IsValueNode() bool {


Can be unexported again, it's not used anywhere outside this file

@schomatis I pushed a commit for this

aschmahmann · 2021-10-21T20:39:44Z

io/directory.go

@@ -316,11 +344,11 @@ func (d *BasicDirectory) GetCidBuilder() cid.Builder {
 }

 // SwitchToSharding returns a HAMT implementation of this directory.
-func (d *BasicDirectory) SwitchToSharding(ctx context.Context) (*HAMTDirectory, error) {
+func (d *BasicDirectory) SwitchToSharding(ctx context.Context, shardWidth int) (*HAMTDirectory, error) {


This is a breaking change that will have to be propagated upstream, but seems reasonable 👍

AFAICT this isn't used by tests or anywhere else. What was the reason to make this change, just to align with hamt.NewShard()?

Yes, we can revert it back if you want.

io/directory.go

- Add tests for automatic unsharding - Modified some internals to be sufficiently extensible for testing

schomatis self-assigned this Jun 23, 2021

schomatis force-pushed the schomatis/directory/unsharding-tests branch from dafe0c3 to 9fbc4b7 Compare June 23, 2021 23:09

schomatis mentioned this pull request Jun 23, 2021

Size-based unsharding #94

Merged

4 tasks

schomatis force-pushed the schomatis/directory/unsharding-tests branch 2 times, most recently from 912b849 to 790373d Compare June 24, 2021 15:29

schomatis force-pushed the schomatis/directory/unsharding branch from 068bcdd to 3301178 Compare June 29, 2021 19:42

schomatis mentioned this pull request Aug 3, 2021

Tracking issue for UnixFS automatic sharding ipfs/kubo#8106

Closed

schomatis force-pushed the schomatis/directory/unsharding-tests branch from 445e541 to 444e5b8 Compare August 11, 2021 19:09

aschmahmann changed the base branch from schomatis/directory/unsharding to schomatis/directory/unsharding-split/add-unshadring-for-addchild August 12, 2021 01:31

schomatis mentioned this pull request Aug 12, 2021

Remove tiemout and refactor sizeBelowThreshold #102

Merged

schomatis requested review from aschmahmann and Stebalien August 13, 2021 19:29

aschmahmann reviewed Aug 25, 2021

View reviewed changes

schomatis force-pushed the schomatis/directory/unsharding-split/add-unshadring-for-addchild branch 3 times, most recently from 7a7204f to 16a96a3 Compare August 26, 2021 16:42

Base automatically changed from schomatis/directory/unsharding-split/add-unshadring-for-addchild to schomatis/directory/unsharding August 26, 2021 16:44

schomatis force-pushed the schomatis/directory/unsharding-tests branch from 599c09e to 4568234 Compare August 27, 2021 18:34

unsharding tests

d60be37

schomatis force-pushed the schomatis/directory/unsharding-tests branch from 4568234 to d60be37 Compare August 27, 2021 18:35

schomatis added 2 commits August 27, 2021 15:42

go fmt

c5e0a00

go static check

2713c85

schomatis requested a review from aschmahmann August 27, 2021 18:52

schomatis mentioned this pull request Aug 30, 2021

[unsharding] Follow up on some low-priority fixes/renames/refactors #105

Closed

8 tasks

aschmahmann reviewed Aug 31, 2021

View reviewed changes

io/directory_test.go Outdated Show resolved Hide resolved

io/directory_test.go Outdated Show resolved Hide resolved

io/directory_test.go Outdated Show resolved Hide resolved

schomatis added 3 commits August 31, 2021 16:24

TestProductionLinkSize

8a80e74

fix dep listing

94a6082

TestProductionLinkSize: fix context

2f9afe9

schomatis mentioned this pull request Sep 1, 2021

Concurrent walk test #106

Merged

Concurrent walk test (#106)

cdcb270

* wip * fix CreateCompleteHAMT test * relax test constraint * fix value of nodes fetched in test according to a BFS walk

schomatis added 4 commits October 14, 2021 11:51

fixmes

2c49dc3

reduce use of DefaultShardWidth to production

8176abf

rework TestHAMTEnumerationWhenComputingSize documentation

9795f4e

clean up dead code

805db3b

schomatis marked this pull request as ready for review October 14, 2021 15:32

go fmt

32ccacd

aschmahmann mentioned this pull request Oct 14, 2021

create internal and private packages #108

Merged

schomatis added 3 commits October 15, 2021 18:58

comments

dc7e405

fix TestHAMTEnumerationWhenComputingSize numbers

ba5e036

aschmahmann reviewed Oct 21, 2021

View reviewed changes

unexport shard.IsValueNode and remove shard.EnumAll

ed5213a

aschmahmann reviewed Oct 21, 2021

View reviewed changes

move utilities for directory testing into the test file

860cb96

aschmahmann reviewed Oct 21, 2021

View reviewed changes

io/directory.go Outdated Show resolved Hide resolved

aschmahmann and others added 5 commits October 21, 2021 16:46

remove EnumAllAsync

0e831ed

revert switch to sharding to use default

ba2d000

remove leftover code

f7a8e99

remove commented out code

20d45d6

move completehamt code into testing

64502b0

aschmahmann approved these changes Oct 22, 2021

View reviewed changes

aschmahmann merged commit 51cb5fe into schomatis/directory/unsharding Oct 22, 2021

schomatis deleted the schomatis/directory/unsharding-tests branch October 22, 2021 21:21

schomatis added a commit that referenced this pull request Oct 22, 2021

Tests for unsharding PR (#99)

9ea81f2

- Add tests for automatic unsharding - Modified some internals to be sufficiently extensible for testing

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tests for unsharding PR #99

Tests for unsharding PR #99

schomatis commented Jun 23, 2021 •

edited

Loading

aschmahmann left a comment

aschmahmann Aug 12, 2021

aschmahmann Aug 25, 2021

aschmahmann Oct 21, 2021

aschmahmann Aug 25, 2021

schomatis Aug 27, 2021

aschmahmann Oct 12, 2021

schomatis Oct 14, 2021

aschmahmann Oct 21, 2021

aschmahmann Oct 21, 2021

aschmahmann Oct 21, 2021

aschmahmann Oct 21, 2021

aschmahmann Oct 21, 2021

aschmahmann Oct 21, 2021

schomatis Oct 22, 2021

Tests for unsharding PR #99

Tests for unsharding PR #99

Conversation

schomatis commented Jun 23, 2021 • edited Loading

aschmahmann left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schomatis commented Jun 23, 2021 •

edited

Loading