geode-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From John Blum <jb...@pivotal.io>
Subject Re: [DISCUSS] changes to registerInterest API
Date Thu, 30 Nov 2017 22:33:37 GMT
For instance, this came up recently...

https://stackoverflow.com/questions/46551278/gemfire-composite-key-pojo-as-gemfire-key

I have seen other similar posts too!



On Thu, Nov 30, 2017 at 2:30 PM, John Blum <jblum@pivotal.io> wrote:

> Does anyone actually do this in practice?  If so, yikes!
>
> Even if the List is immutable, the elements may not be, so using a List as
> a key starts to open 1 up to a lot of problems.
>
> As others have pointed out in SO and other channels, information should
> not be kept in the key.
>
> It is perfect fine to have a "Composite" Key, but then define a
> CompositeKey class type with properly implemented equals(:Object) and
> hashCode():int methods.
>
> For the most part, Keys should really only ever be simple Scalar values
> (e.g. Long, String, etc).
>
> -j
>
>
>
>
> On Thu, Nov 30, 2017 at 2:25 PM, Jason Huynh <jhuynh@pivotal.io> wrote:
>
>> I started work on the following plan:
>> - deprecate current "ALL_KEYS" and List passing behavior in
>> registerInterest
>> ()
>> - add registerInterestForAllKeys();
>> - add registerInterest(T... keys)
>> - add registerInterest(Iterable<T>keys)
>>
>> I might be missing something here but:
>> With the addition of registerInterest(Iterable<T> keys), I think we would
>> not be able to register interest a List as the key itself.  A list would
>> be
>> iterated over due to the addition of registerInterest(Iterable<T> keys).
>> A
>> list in a list would be passed into registerInterest and again be iterated
>> over.  I could change the newly created registerInterest call and
>> explicitly name it something else or are we ok with Iterables not being
>> able to be registered as individual keys.
>>
>>
>>
>>
>>
>> On Mon, Nov 20, 2017 at 9:05 AM Kirk Lund <klund@apache.org> wrote:
>>
>> > John's approach looks best for when you need to specify keys.
>> >
>> > For ALL_KEYS, what about an API that doesn't require a token or all
>> keys:
>> >
>> > public void registerInterestForAllKeys();
>> >
>> > On Fri, Nov 17, 2017 at 1:24 PM, Jason Huynh <jhuynh@pivotal.io> wrote:
>> >
>> > > Thanks John for the clarification!
>> > >
>> > > On Fri, Nov 17, 2017 at 1:12 PM John Blum <jblum@pivotal.io> wrote:
>> > >
>> > > > This...
>> > > >
>> > > > > The Iterable version would handle any collection type by having
>> the
>> > > user
>> > > > pass
>> > > > in the iterator for the collection.
>> > > >
>> > > > Is not correct.
>> > > >
>> > > > The Collection<E> interface itself "extends" the
>> java.lang.Iterable<E>
>> > > > interface (see here...
>> > > > https://docs.oracle.com/javase/8/docs/api/java/util/Collection.html
>> > > under
>> > > > "*All
>> > > > Superinterfaces*").
>> > > >
>> > > > Therefore a user can simply to this...
>> > > >
>> > > > *List*<KeyType> keys = ...
>> > > >
>> > > > region.registerInterest(keys); *// calls the
>> > > > Region.registerInterest(:Iterable<T>) method.*
>> > > >
>> > > > Alternatively, this would also be allowed...
>> > > >
>> > > > *Set*<KeyType> keys = ...
>> > > >
>> > > > region.registerInterest(keys);
>> > > >
>> > > >
>> > > > On Fri, Nov 17, 2017 at 11:44 AM, Jason Huynh <jhuynh@pivotal.io>
>> > wrote:
>> > > >
>> > > > > Current idea is to:
>> > > > > - deprecate current "ALL_KEYS" and List passing behavior in
>> > > > > registerInterest()
>> > > > > - add registerInterestAllKeys();
>> > > > > - add registerInterest(T... keys) and registerInterest(Iterable<T>
>> > > keys)
>> > > > > and
>> > > > > not have one specifically for List or specific collections.
>> > > > >
>> > > > > The Iterable version would handle any collection type by having
>> the
>> > > user
>> > > > > pass in the iterator for the collection.
>> > > > >
>> > > > > On Fri, Nov 17, 2017 at 11:32 AM Jacob Barrett <
>> jbarrett@pivotal.io>
>> > > > > wrote:
>> > > > >
>> > > > > > I am failing to see where registerInterest(List<T>
keys) is an
>> > issue
>> > > > for
>> > > > > > the key type in the region. If our region is Region<String>
>> then I
>> > > > would
>> > > > > > expect registerInterest(List<String>). If the keys
are unknown
>> or a
>> > > mix
>> > > > > > then you should have Region<Object> and thus
>> > > > > registerInterest(List<Object).
>> > > > > >
>> > > > > > I echo John's statements on VarArgs and type erasure as
well as
>> his
>> > > > > > argument for Iterable<T>.
>> > > > > >
>> > > > > > Also, List<T> does not restrict you from List indexes.
The
>> region
>> > > would
>> > > > > be
>> > > > > > Region<List<String>> with registerInterest<List<List<Str
>> ing>>().
>> > > > > >
>> > > > > > -Jake
>> > > > > >
>> > > > > >
>> > > > > > On Fri, Nov 17, 2017 at 10:04 AM John Blum <jblum@pivotal.io>
>> > wrote:
>> > > > > >
>> > > > > > > Personally, I prefer the var args method
>> (registerInterest(T...
>> > > > keys))
>> > > > > > > myself.  It is way more convenient if I only have a
few keys
>> when
>> > > > > calling
>> > > > > > > this method then to have to add the keys to a List,
especially
>> > for
>> > > > > > testing
>> > > > > > > purposes.
>> > > > > > >
>> > > > > > > But, I typically like to pair that with a
>> > > > registerInterest(Iterable<T>
>> > > > > > > keys) method
>> > > > > > > as well.  By having a overloaded Iterable variant,
then I can
>> > pass
>> > > in
>> > > > > any
>> > > > > > > Collection type I want (which shouldn't be restricted
to just
>> > > List).
>> > > > > It
>> > > > > > > also is a simple matter to convert any *Collection*
(i.e.
>> *List*,
>> > > > > *Set*,
>> > > > > > > etc) to an array, which can be passed to the var args
>> method.  By
>> > > > using
>> > > > > > > List,
>> > > > > > > you are implying that "order matters" since a List
is a order
>> > > > > collection
>> > > > > > of
>> > > > > > > elements.
>> > > > > > >
>> > > > > > > This ("*It might even cause problems of pushing in
**multiple
>> > > > different
>> > > > > > > types.*"), regarding var args, does not even make sense.
>> > > Technically,
>> > > > > > > List<T> is no different.  Java's type erasure
essentially
>> equates
>> > > var
>> > > > > > args
>> > > > > > > too "Object..." (or Object[]) and the List<T>
to List (or a
>> List
>> > of
>> > > > > > > Objects,
>> > > > > > > essentially like if you just did this... List<Object>)
So,
>> while
>> > > the
>> > > > > > > compiler ensures compile-time type-safety of generics,
there
>> is
>> > no
>> > > > > > generics
>> > > > > > > type-safety guarantees at runtime.
>> > > > > > >
>> > > > > > >
>> > > > > > >
>> > > > > > > On Fri, Nov 17, 2017 at 9:22 AM, Jason Huynh <
>> jhuynh@pivotal.io>
>> > > > > wrote:
>> > > > > > >
>> > > > > > > > Hi Mike,
>> > > > > > > >
>> > > > > > > > The current support for List leads to compilation
issues if
>> the
>> > > > > region
>> > > > > > is
>> > > > > > > > type constrained.  However I think you are suggesting
>> instead
>> > of
>> > > a
>> > > > > var
>> > > > > > > args
>> > > > > > > > method, instead provide a registerInterest(List
keys)
>> method?
>> > > > > > > >
>> > > > > > > > So far what I am hearing requested is:
>> > > > > > > > deprecate current "ALL_KEYS" and List passing
behavior
>> > > > > > > > registerInterestAllKeys();
>> > > > > > > > registerInterest(List<T> keys) instead of
a
>> > registerInterest(T...
>> > > > > keys)
>> > > > > > > >
>> > > > > > > > Will anyone ever actually have a List as the key
itself? The
>> > > > current
>> > > > > > and
>> > > > > > > > suggested changes would not allow it registering
for a
>> specific
>> > > > List
>> > > > > > > > object.
>> > > > > > > >
>> > > > > > > >
>> > > > > > > >
>> > > > > > > > On Thu, Nov 16, 2017 at 6:50 PM Jacob Barrett
<
>> > > jbarrett@pivotal.io
>> > > > >
>> > > > > > > wrote:
>> > > > > > > >
>> > > > > > > > > Geode Native C++ and .NET have:
>> > > > > > > > >
>> > > > > > > > >   virtual void registerKeys(const
>> > > > > > > > > std::vector<std::shared_ptr<CacheableKey>>
& keys,
>> > > > > > > > >                             bool isDurable
= false,
>> > > > > > > > >                             bool getInitialValues
= false,
>> > > > > > > > >                             bool receiveValues
= true) =
>> 0;
>> > > > > > > > >
>> > > > > > > > >   virtual void unregisterKeys(const
>> > > > > > > > > std::vector<std::shared_ptr<CacheableKey>>
& keys) = 0;
>> > > > > > > > >
>> > > > > > > > >   virtual void *registerAllKeys*(bool isDurable
= false,
>> > > > > > > > >                                bool getInitialValues
=
>> false,
>> > > > > > > > >                                bool receiveValues
= true)
>> =
>> > 0;
>> > > > > > > > >
>> > > > > > > > >   virtual void unregisterAllKeys() = 0;
>> > > > > > > > >
>> > > > > > > > >   virtual void registerRegex(const std::string&
regex,
>> > > > > > > > >                              bool isDurable
= false,
>> > > > > > > > >                              bool getInitialValues
=
>> false,
>> > > > > > > > >                              bool receiveValues
= true) =
>> 0;
>> > > > > > > > >
>> > > > > > > > >   virtual void unregisterRegex(const char*
regex) = 0;
>> > > > > > > > >
>> > > > > > > > > I dislike special values like this so yes
please make it
>> go
>> > > away!
>> > > > > > > > >
>> > > > > > > > > -Jake
>> > > > > > > > >
>> > > > > > > > >
>> > > > > > > > > On Thu, Nov 16, 2017 at 5:20 PM Dan Smith
<
>> dsmith@pivotal.io
>> > >
>> > > > > wrote:
>> > > > > > > > >
>> > > > > > > > > > I don't really like the regex option
- it implies that
>> your
>> > > > keys
>> > > > > > are
>> > > > > > > > all
>> > > > > > > > > > strings. Will any other regular expressions
work on non
>> > > string
>> > > > > > > objects?
>> > > > > > > > > > registerInterestAllKeys() seems like
a better option.
>> > > > > > > > > >
>> > > > > > > > > > -Dan
>> > > > > > > > > >
>> > > > > > > > > > On Thu, Nov 16, 2017 at 4:34 PM, Michael
Stolz <
>> > > > > mstolz@pivotal.io>
>> > > > > > > > > wrote:
>> > > > > > > > > >
>> > > > > > > > > > > I don't like the vararg option.
>> > > > > > > > > > > If i'm maintaining a list of keys
i'm interested in, I
>> > want
>> > > > to
>> > > > > be
>> > > > > > > > able
>> > > > > > > > > to
>> > > > > > > > > > > pass that List in.
>> > > > > > > > > > > Varargs is a poor substitute. It
might even cause
>> > problems
>> > > of
>> > > > > > > pushing
>> > > > > > > > > in
>> > > > > > > > > > > multiple different types. Keys
must all be of one type
>> > for
>> > > a
>> > > > > > given
>> > > > > > > > > > Region.
>> > > > > > > > > > >
>> > > > > > > > > > >
>> > > > > > > > > > > I'm very much in favor of deprecating
the ALL_KEYS
>> string
>> > > in
>> > > > > > favor
>> > > > > > > of
>> > > > > > > > > > > something that is typed specially
if you refer to
>> > ALL_KEYS.
>> > > > > > > > > > >
>> > > > > > > > > > >
>> > > > > > > > > > > If that works, then we don't necessarily
need the
>> > > additional
>> > > > > API
>> > > > > > > > > > > registerInterestAllKeys(). But
if ALL_KEYS can't be a
>> > > special
>> > > > > > type
>> > > > > > > to
>> > > > > > > > > get
>> > > > > > > > > > > over the compilation issues then
we should go with the
>> > new
>> > > > API.
>> > > > > > > > > > >
>> > > > > > > > > > >
>> > > > > > > > > > >
>> > > > > > > > > > > --
>> > > > > > > > > > > Mike Stolz
>> > > > > > > > > > > Principal Engineer, GemFire Product
Lead
>> > > > > > > > > > > Mobile: +1-631-835-4771 <(631)%20835-4771>
>> > <(631)%20835-4771>
>> > > > <(631)%20835-4771> <(631)%20835-4771>
>> > > > > > <(631)%20835-4771>
>> > > > > > > <(631)%20835-4771>
>> > > > > > > > > > >
>> > > > > > > > > > > On Thu, Nov 16, 2017 at 7:02 PM,
Anilkumar Gingade <
>> > > > > > > > > agingade@pivotal.io>
>> > > > > > > > > > > wrote:
>> > > > > > > > > > >
>> > > > > > > > > > > > +1 Deprecating ALL_KEYS option;
I believe this is
>> added
>> > > > > before
>> > > > > > we
>> > > > > > > > > > > supported
>> > > > > > > > > > > > regex support.
>> > > > > > > > > > > >
>> > > > > > > > > > > >  Doesn't seems like a new
API is needed. The regex
>> java
>> > > doc
>> > > > > > > clearly
>> > > > > > > > > > > > specifies the effect of ".*".
>> > > > > > > > > > > >
>> > > > > > > > > > > > +1 for deprecating list argument;
and replacing with
>> > new
>> > > > API.
>> > > > > > > > > > > >
>> > > > > > > > > > > > -Anil.
>> > > > > > > > > > > >
>> > > > > > > > > > > >
>> > > > > > > > > > > >
>> > > > > > > > > > > > On Thu, Nov 16, 2017 at 3:36
PM, Jason Huynh <
>> > > > > > jhuynh@pivotal.io>
>> > > > > > > > > > wrote:
>> > > > > > > > > > > >
>> > > > > > > > > > > > > For GEODE-3813 <
>> > > > > > > https://issues.apache.org/jira/browse/GEODE-3813
>> > > > > > > > >:
>> > > > > > > > > > > > Region
>> > > > > > > > > > > > > registerInterest API
usage of type parameters is
>> > broken
>> > > > > > > > > > > > > <https://issues.apache.org/jira/browse/GEODE-3813
>> >
>> > > > > > > > > > > > >
>> > > > > > > > > > > > >
>> > > > > > > > > > > > > The current API to registerInterest
allows a
>> special
>> > > > string
>> > > > > > > token
>> > > > > > > > > > > > > “ALL_KEYS” to be
passed in as the parameter to
>> > > > > > > registerInterest(T
>> > > > > > > > > > key).
>> > > > > > > > > > > > > This special token causes
the registerInterest to
>> > > behave
>> > > > > > > similar
>> > > > > > > > to
>> > > > > > > > > > > > > registerInterestRegex(“.*”).
 As the ticket
>> states,
>> > if
>> > > > the
>> > > > > > > region
>> > > > > > > > > has
>> > > > > > > > > > > > been
>> > > > > > > > > > > > > typed to anything other
than Object or String, the
>> > > usage
>> > > > of
>> > > > > > > > > > “ALL_KEYS”
>> > > > > > > > > > > > as a
>> > > > > > > > > > > > > parameter results in
a compilation error.
>> > > > > > > > > > > > >
>> > > > > > > > > > > > >
>> > > > > > > > > > > > > Proposals:
>> > > > > > > > > > > > >
>> > > > > > > > > > > > > I would like to deprecate
the special string
>> > “ALL_KEYS”
>> > > > and
>> > > > > > > > > document
>> > > > > > > > > > a
>> > > > > > > > > > > > > workaround of using registerInterestRegex(“.*”)
>> or we
>> > > can
>> > > > > > add a
>> > > > > > > > new
>> > > > > > > > > > API
>> > > > > > > > > > > > > called registerInterestAllKeys()
>> > > > > > > > > > > > >
>> > > > > > > > > > > > >
>> > > > > > > > > > > > > I think we should also
deprecate passing a List
>> > Object
>> > > of
>> > > > > > keys
>> > > > > > > > into
>> > > > > > > > > > > > > registerInterest.  It
has the same compilation
>> > > > restrictions
>> > > > > > as
>> > > > > > > > > > > “ALL_KEYS”
>> > > > > > > > > > > > > when the region is key
constrained/typed.  The
>> reason
>> > > why
>> > > > > > List
>> > > > > > > > > would
>> > > > > > > > > > be
>> > > > > > > > > > > > > used is to allow registering
multiple keys at
>> once.
>> > > > > Instead,
>> > > > > > > we
>> > > > > > > > > can
>> > > > > > > > > > > add
>> > > > > > > > > > > > a
>> > > > > > > > > > > > > new var arg API like
registerInterest(T… keys).
>> This
>> > > > > problem
>> > > > > > > and
>> > > > > > > > > > > > solution
>> > > > > > > > > > > > > was also documented in
the ticket by the ticket
>> > creator
>> > > > > (Kirk
>> > > > > > > > Lund)
>> > > > > > > > > > > > >
>> > > > > > > > > > > > >
>> > > > > > > > > > > > >
>> > > > > > > > > > > > > Thanks,
>> > > > > > > > > > > > >
>> > > > > > > > > > > > > -Jason
>> > > > > > > > > > > > >
>> > > > > > > > > > > >
>> > > > > > > > > > >
>> > > > > > > > > >
>> > > > > > > > >
>> > > > > > > >
>> > > > > > >
>> > > > > > >
>> > > > > > >
>> > > > > > > --
>> > > > > > > -John
>> > > > > > > john.blum10101 (skype)
>> > > > > > >
>> > > > > >
>> > > > >
>> > > >
>> > > >
>> > > >
>> > > > --
>> > > > -John
>> > > > john.blum10101 (skype)
>> > > >
>> > >
>> >
>>
>
>
>
> --
> -John
> john.blum10101 (skype)
>



-- 
-John
john.blum10101 (skype)

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message