Make Batch implement a NodeAdder interface

[hsanjuan]

Needed for: https://github.com/ipfs/go-unixfs/pull/41

[Stebalien]

Personally, I think separating out a NodeWriter interface is probably the right way to go.

[hsanjuan]

@Stebalien you mean NodeWriter being an interface with a single Add method, and using NodeWriter directly in places which only Add() (importers) instead of a full featured DAGService (and Batch would implement it) ?

[Stebalien]

type NodeWriter interface {
    Add(context.Context, Node) error
    AddMany(context.Context, []Node) error
}

(we'd also need Batch to implement AddMany but that shouldn't be difficult).

We don't have to do it this way, I just don't want batch.Add(ctx, x); batch.Get(ctx, x.Cid()) to fail.

(as names, NodeWriter is even worse than NodeGetter...)

[hsanjuan]

So it's one more interface to the mix (NodeAdder ?) vs. having a suboptimal DAGService implementation for methods we don't use right now.

Before, doing Get on the DAGService while adding stuff with the batch-importers would cause the same problems (Get on a recently added node could fail it was still uncommitted).

I'm leaning towards just doing Commit before non-add operations and documenting that Batch does it this way and if someone ever have an interest in improving it can always be done, but I'm honestly not sure what's best (both work, both are easy to implement).

[Stebalien]

having a suboptimal DAGService implementation for methods we don't use right now.

No, having a broken DAGService implementation that sometimes works and sometimes doesn't. It doesn't matter if we're not using it, the issue is that it looks like it should work but won't.

Before, doing Get on the DAGService while adding stuff with the batch-importers would cause the same problems (Get on a recently added node could fail it was still uncommitted).

Yes, but that makes perfect sense. The "batch" is effectively a transaction. However, if we're going to claim that Batch implements DAGService, it needs to actually implement DAGService.

I'm leaning towards just doing Commit before non-add operations and documenting that Batch does it this way and if someone ever have an interest in improving it can always be done, but I'm honestly not sure what's best (both work, both are easy to implement).

We can, of course, do that. However, I really don't see the issue adding a new interface. It gives us a clean divide so applications can pick what they want to use:

• NodeGetter - read-only
• NodeAdder - append-only
• DAGService - RW dag service

[hsanjuan]

No, having a broken DAGService...

I meant with the Commit before Get fix etc. It's suboptimal because it may make Gets really slow/defeat the purpose of batching.

However, I really don't see the issue adding a new interface

Ok i'll do that then

[Stebalien]

I meant with the Commit before Get fix etc. It's suboptimal because it may make Gets really slow/defeat the purpose of batching.

Ah, got it. Really, either way is fine with me as long as whatever we do is correct. However, this is really like the io.Reader/io.Writer split so I think it'll be fine.

[hsanjuan]

Have a look to latest commit (https://github.com/ipfs/go-ipld-format/pull/46/commits/cfe76e8fb58636697a9743313bb9afe758c3c3ed) @Stebalien

I still need to update the gx tag before merging.

[Stebalien]

@hsanjuan LGTM.