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.

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

add convenience methods for creating new maps, sets, and counters #4

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

hopkinsth
Copy link

We use several nested maps inside our app, and it would be more convenient to pass around just the map itself to different functions instead of the map and the crdt struct pointer. This isn't a critical problem, but it's a little more convenient to have a NewMap, NewSet, and NewCounter function available on map structs.

The other options for implementing this include stuff like:

  1. exposing the unexported crdt field on map structs
  2. adding an anonymous Crdt field on maps

But both those options seem like they create a weird API, so this made most sense to me.

Review on Reviewable

@boj
Copy link
Member

boj commented Nov 24, 2014

The current implementation is definitely a little crude.

I currently have something like this:

type TaskMap struct {
    crdt *riaken.Crdt
}

func NewTaskMap(c *Context, id string) *TaskMap {
    tm := &TaskMap{}

    bucket := c.B(BUCKET_TASK_MAPS).Type(BUCKET_TYPE_MAPS)
    tm.crdt = bucket.Crdt(id)
    tm.crdt.Map = tm.crdt.NewMap()
    tm.crdt.Map.Sets["approvers"] = tm.crdt.NewSet()
    tm.crdt.Map.Sets["comments"] = tm.crdt.NewSet()

    return tm
}

Maybe something like the following would be better?

bucket := c.B(BUCKET_TASK_MAPS).Type(BUCKET_TYPE_MAPS)
tm.crdt = bucket.Crdt(id)
tm.crdt.Map.NewSet("approvers")
tm.crdt.Map.NewSet("comments")
tm.crdt.Map.NewCounter("somecounter")
tm.crdt.Map.NewMap("somemap").NewSet("someset")

@boj
Copy link
Member

boj commented Nov 26, 2014

@hopkinsth If you could write some tests which utilize your convenience functions I'll merge this in. Idiomatic method documentation would be nice too.

Example:

// NewMap returns a new map.
func (m *CrdtMap) NewMap() *CrdtMap {

@hopkinsth
Copy link
Author

I apologize for not responding sooner—lots going on this week.

I was originally thinking something like this...

mp, err := bucket.CrdtMap("my-key-here")
if err != nil {
  switch err.(type) {
    case riaken_core.WRONG_CRDT_TYPE:
      fmt.Println("Key does not match requested type!")
      //or something like that
    default:
      fmt.Println("some other error?")
  }
}

innerMap, err := mp.Map("my-map-key")
if err != nil {
  //more "key doesn't match type" or something like that
  //also "map not found, maybe?"
  //error on duplicate keys? 
  //return old map on duplicate keys? (ew.)
}

innerSet, err := mp.Set("my-set-key")
//etc. 

That adds a fair amount of complexity and, with it, probably decreases performance.

I'll push up a test and doc comments for the three methods in this PR soon. They would be handy to have.

@hopkinsth
Copy link
Author

I pushed up a test for these methods, which just checks that all nested maps, counters, and sets refer to the same Crdt struct pointer. My new test passes, but I'm getting some errors from other tests:

--- FAIL: TestCrdtCounter (0.00 seconds)
    crdt_test.go:18: riak error [0]: Bucket datatype `undefined` is not a supported type
--- FAIL: TestCrdtSet (0.00 seconds)
    crdt_test.go:84: riak error [0]: Bucket datatype `undefined` is not a supported type
--- FAIL: TestCrdtMap (0.00 seconds)
    crdt_test.go:166: riak error [0]: Bucket datatype `undefined` is not a supported type
--- FAIL: TestQuerySecondaryIndexes (0.01 seconds)
    query_test.go:133: riak error [0]: {error,{indexes_not_supported,riak_kv_bitcask_backend}}
    query_test.go:137: expected results
--- FAIL: TestQuerySearch (0.00 seconds)
    query_test.go:168: riak error [0]: Unknown message code: 27
    query_test.go:172: expected results
--- FAIL: TestQuerySearchCompound (0.00 seconds)
    query_test.go:199: riak error [0]: Unknown message code: 27
    query_test.go:203: expected results
FAIL
exit status 1
FAIL    github.com/riaken/riaken-core   0.148s

I'm fairly certain these errors and my change aren't related, but let me know if something looks weird. Not sure yet what the first set of errors are about. I'm running these tests locally with Riak 2.0.1.

@boj
Copy link
Member

boj commented Nov 27, 2014

@hopkinsth Assuming your database is local run make test-prep to set the correct bucket properties on your database.

The 2i test won't pass unless your backend is set to leveldb.

The two 1.4 Search's are expected (for now).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants