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

PivotedStore #186

Merged
merged 7 commits into from
Nov 21, 2013
Merged

PivotedStore #186

merged 7 commits into from
Nov 21, 2013

Conversation

rubanm
Copy link
Contributor

@rubanm rubanm commented Nov 16, 2013

Addresses #178

@rubanm
Copy link
Contributor Author

rubanm commented Nov 16, 2013

This is a fun one.

@johnynek Can you take a peek? This change also adds an IterableStore since it looks like get would involve fetching all underlying keys that "contain" the outer key. Let me know if this makes sense. Thanks.

Tests TBD

*/
trait IterableReadableStore[-K, V] extends ReadableStore[K, V] {

def getAll[K1 <: K]: Future[Iterable[(K1, V)]]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In scala-land I like to steer clear of java-bean naming conventions. how about exposing something like IterableReadableStore#iterator ala Map#iterator. getAll also implies you are loading all members into memory. iterables/iterators can imply lazyiness which I like.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. Keeps the method name familiar and less scary.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an object in util called a spool:

https://github.com/twitter/util/blob/master/util-core/src/main/scala/com/twitter/concurrent/Spool.scala

It may be that this returns a Future[Spool[(K1,V)]] which means that the we can make multiple future calls to page the results out.

@rubanm
Copy link
Contributor Author

rubanm commented Nov 16, 2013

@softprops After reading your comment, I'm thinking Iterable might be a useful trait for reading from message queues or streams if we go down that path.

trait IterableSource[K, V] {
  def iterator : Future[Iterable[(K, V)]]
}

trait IterableReadableStore[-K, V] extends ReadableStore[K, V]
  with IterableSource[K, Option[V]]

trait IterableStore[-K, V] extends IterableReadableStore[K, V] with Store[K, V]

I'm not completely clear with the naming here. For sinks, we have kept the existing WriteableStore name, so IterableSource might be confusing.

Thoughts?

* @author Ruban Monu
*/
class PivotedReadableStore[K, -OuterK, InnerK, +V](store: IterableReadableStore[K, V])(bij: Bijection[(OuterK, InnerK), K])
extends ReadableStore[OuterK, Map[InnerK, V]] {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. This is cool, but I doubt many stores we want to use it with can actually be iterated like this.

  2. Here is a second idea: make this ReadableStore[OuterK, ReadableStore[InnerK, V]]. I think that can be done without the Iterable for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, the point on not many stores being iterable is valid.

I'm not completely clear how making this extend ReadableStore[OuterK, ReadableStore[InnerK, V]] would help in avoiding Iterable though. Seems to me like we need some way to know which Ks in the underlying store constitute OuterK and InnerK when executing get(outerK).
Or what am I missing here?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, get(outerK) does nothing but adds a prefix. Then when someone calls .get on the inner store, you build the real keys and make the call.

I don't think you need to enumerate.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I think I understand now :) Thanks.
So something like:

override def get(outerK: OuterK) : Future[Option[ReadableStore[InnerK, V]]] =
  Future.value(Some(ReadableStore[InnerK, V] {
    override def get(innerK: InnerK) {
    val k = inj((outerK, innerK))
    underlying.get(K)
  })
}

Also, this
http://thecodinglove.com/post/51069449556/senior-developer-explaining-how-to-use-his-library

@rubanm
Copy link
Contributor Author

rubanm commented Nov 19, 2013

Posted implementation that uses ReadableStore[OuterK, ReadableStore[InnerK, V]].
A writeable version may still need some kind of enumeration, so I've reverted that for now.

Also removed IterableSource since that's probably going to be its own thing. Will send that out as a separate change.

def fromMap[K, OuterK, InnerK, V](m: Map[K, V])(inj: Injection[(OuterK, InnerK), K]) =
new PivotedReadableStore[K, OuterK, InnerK, V](ReadableStore.fromMap(m))(inj)

def fromReadableStore[K, OuterK, InnerK, V](store: ReadableStore[K, V])(inj: Injection[(OuterK, InnerK), K]) =
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the usual pattern is that the injection be implicit. If you want to call explicitly, you can still do so. What do you think of that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. Made it implicit.

johnynek added a commit that referenced this pull request Nov 21, 2013
@johnynek johnynek merged commit 81ecf6b into twitter:develop Nov 21, 2013
@rubanm rubanm deleted the feature/pivoted_store branch November 22, 2013 04:25
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.

3 participants