search

basho / riak_pb

Riak Protocol Buffers Messages
Apache License 2.0
71 stars 114 forks source link

add gsets support [JIRA: RIAK-2690] #203

Closed binarytemple-external closed 8 years ago

binarytemple-external commented 8 years ago

@russelldb corrected P/R to add gset support

basho/riak_kv#1437

binarytemple-external commented 8 years ago

TODO: Address issues identified by @lukebakken in https://github.com/basho/riak_pb/pull/202

DSomogyi commented 8 years ago

create jira issue

lukebakken commented 8 years ago

I spent some time throwing together a solution that would re-use existing message fields and not duplicate code. Basically, on DtUpdateReq it requires the addition of a is_gset boolean. Pretty much that and the addition of the GSET type is all that is necessary. gset values would be returned in set_value and the type field of DtFetchResp would be used to determine the type of set_value. Having both a type discriminator and two different fields for that type is redundant.

@zeeshanlakhani @russelldb @binarytemple-bet365 thoughts?

zeeshanlakhani commented 8 years ago

@lukebakken got a commit/gist to look at?

lukebakken commented 8 years ago

@zeeshanlakhani @binarytemple-bet365 see #204 (RIAK-1971) (RIAK-2250) (RIAK-2672)

binarytemple-external commented 8 years ago

@zeeshanlakhani @russelldb @lukebakken - I think it seems brittle. OTOH - I stand here in the role of supplicant before this temple of code. Let me know your decision and I'll comply.

binarytemple-external commented 8 years ago

Here's a diff comparison for convenience : https://github.com/bet365/riak_pb/compare/feature/gsets...basho:features/lrb/add-gset-support

lukebakken commented 8 years ago

I closed my PR after reading the scroll back in the room.

Having a GSET discriminator and gset_value field is redundant. I would like to see gset_value removed. You're only every going to return one thing that is a set (no matter how many different sets are implemented), and the one field suffices.