Jenkins build is back to normal : Geode-release #90

[Apache Jenkins Server]

See 

Re: [DISCUSS] Addition of isValid API to Index interface

[Nabarun Nag]

Thanks you guys for the review. I will revert the GEODE-3520 ticket to
reflect that invalidate should happen for both synchronous and asynchronous
index maintenance.
Also, I will resend the PR which reflects the changes mentioned in the
ticket

Regards
Nabarun Nag





On Thu, Sep 14, 2017 at 5:55 PM Anilkumar Gingade 
wrote:

> Dan, you are right...Thanks to Jason, myself and Jason looked into the code
> to see if index is updated before the event/change is saved/injected into
> the region...It looks like the index update are happening after the region
> change/update is saved. Moving the index update before that is not an easy
> task...
>
> For time, when there is any problem with index update, we can proceed with
> invalidating the indexes...But we really need to look at making region and
> index updates in a transactional way, silently invalidating indexes may not
> be acceptable...
>
> -Anil.
>
>
>
>
> On Thu, Sep 14, 2017 at 1:12 PM, Dan Smith  wrote:
>
> > I'm still going to push that we stick with Naba's original proposal.
> >
> > The current behavior is clearly broken. If one index update fails, an
> > exception gets thrown to the user (nice!) but it leaves the put in a
> > partially completed state - some other indexes may not have been updated,
> > WAN/AEQs may not have been notified, etc.
> >
> > We should never leave the system in this corrupted state. It would be
> nice
> > to be able to cleanly rollback the put, but we don't have that capability
> > especially considering that cache writers have already been invoked. So
> the
> > next best thing is to invalidate the index that failed to update.
> >
> > Logging an error an allowing the put to succeed does match what we do
> with
> > CacheListeners. Exceptions from CacheListeners do not fail the put.
> >
> > -Dan
> >
> > On Mon, Sep 11, 2017 at 3:29 PM, Jason Huynh  wrote:
> >
> > > Anil, we actually do have a case where the index is out of sync with
> the
> > > region currently.  It's just not likely to happen but if there is an
> > > exception from an index, the end result is that certain indexes get
> > updated
> > > and the region has already been updated.
> > > However the exception is thrown back to the putter, so it becomes very
> > > obvious something is wrong but I believe Naba has updated the ticket to
> > > show a test that reproduces the problem...
> > >
> > >
> > > On Mon, Sep 11, 2017 at 2:50 PM Anilkumar Gingade  >
> > > wrote:
> > >
> > > > The other way to look at it is; what happens to a cache op; when
> there
> > is
> > > > an exception after Region.Entry is created? can it happen? In that
> > case,
> > > do
> > > > we stick the entry into the Cache or not? If an exception is handled,
> > how
> > > > is it done, can we look at using the same for Index...
> > > >
> > > > Also previously, once the valid index is created (verified during
> > create
> > > or
> > > > first put into the cache); we never had any issue where index is out
> of
> > > > sync with cache...If that changes with new futures (security?) then
> we
> > > may
> > > > have to change the expectation with indexing...
> > > >
> > > > -Anil.
> > > >
> > > >
> > > >
> > > > On Mon, Sep 11, 2017 at 2:16 PM, Anthony Baker 
> > > wrote:
> > > >
> > > > > I’m confused.  Once a cache update has been distributed to other
> > > members
> > > > > it can’t be undone.  That update could have triggered myriad other
> > > > > application behaviors.
> > > > >
> > > > > Anthony
> > > > >
> > > > > > On Sep 11, 2017, at 2:04 PM, Michael Stolz 
> > > wrote:
> > > > > >
> > > > > > Great, that's exactly the behavior I would expect.
> > > > > >
> > > > > > Thanks.
> > > > > >
> > > > > > --
> > > > > > Mike Stolz
> > > > > > Principal Engineer, GemFire Product Manager
> > > > > > Mobile: +1-631-835-4771 <(631)%20835-4771>
> > > > > >
> > > > > > On Mon, Sep 11, 2017 at 4:34 PM, Jason Huynh 
> > > > wrote:
> > > > > >
> > > > > >> Hi Mike, I think the concern was less about the security portion
> > but
> > > > > rather
> > > > > >> if any exception occurs during index update, right now, the
> region
> > > > gets
> > > > > >> updated and the rest of the system (index/wan/callbacks) may or
> > may
> > > > not
> > > > > be
> > > > > >> updated.  I think Naba just tried to provide an example where
> this
> > > > might
> > > > > >> occur, but that specific scenario is invalid.
> > > > > >>
> > > > > >> I believe Nabarun has opened a ticket for rolling back the put
> > > > operation
> > > > > >> when an index exception occurs. GEODE-3589.  It can probably be
> > > > > modified to
> > > > > >> state any exception instead of index exceptions.
> > > > > >>
> > > > > >> To summarize my understanding:
> > > > > >> -Someone will need to implement the rollback for GEODE-3589.
> This
> > > > means
> > > > > >> that if any exception occurs during a put, geode it 

Re: [Discuss] Investigation of C++ object return types

[Jacob Barrett]

Y'all here is an attempt to get the best of both worlds.
https://gist.github.com/pivotal-jbarrett/52ba9ec5de0b494368d1c5282ef188ef

I thought I would try posting to Gist but so far not impressed, sorry.

The Gist of it is that we can overcome the thirdpary or transient reference
back to the public Cache instance by keeping a reference to it in the
implementation instance and updating it whenever the move constructor is
called.

The downside is if your run this test it doesn't show RVO kicking in on the
second test where we move the value into a shared pointer. There are a
couple of pitfalls you can stumble into as well by trying to used the
previous instance to access the cache after the move operation, as
illustrated by the "BAD" commented lines.

The upside is the choices this gives the use for ownership of their factory
constructed Cache instance. They can keep it a value or move it to unique
or shared pointer.

Overhead wise I think we better off in value as long as there are no moves,
rare I would thing, but the moves are pretty cheap at the point since we
only maintain a unique_ptr. After moving into a shared_ptr it acts
identical to the shared_ptr model proposed earlier.

-Jake


On Thu, Sep 14, 2017 at 3:36 PM Michael Martell  wrote:

> Late to this party.
>
> Confession 1: I had to look up both RVO and copy-elision.
> Confession 2: I don't like using pointers at all. I used to, but I've
> evolved to just use C# and Java :)
>
> Without investing a lot more time, I don't have strong feelings about raw
> vs shared pointers. My only question is: Can we return ptr to abstract
> class everywhere we return objects? Just thinking of mocking, which always
> wants to mock interfaces.
>
> On Thu, Sep 14, 2017 at 2:25 PM, Michael William Dodge 
> wrote:
>
> > +0 shared pointer
> >
> > > On 14 Sep, 2017, at 14:09, Ernest Burghardt 
> > wrote:
> > >
> > > Calling a vote for:
> > >
> > > - Raw pointer
> > > - shard pointer
> > >
> > > +1 raw Pointer, I had to look up RVO and am new to std::move(s)
> > >
> > > On Thu, Sep 14, 2017 at 3:02 PM, Michael William Dodge <
> > mdo...@pivotal.io>
> > > wrote:
> > >
> > >> I generally dig reference-counted pointers for avoiding lifetime
> issues
> > >> with objects allocated off the heap but I can live with bare pointers,
> > too.
> > >>
> > >> Sarge
> > >>
> > >>> On 13 Sep, 2017, at 16:25, Mark Hanson  wrote:
> > >>>
> > >>> Hi All,
> > >>>
> > >>> I favor the “pointer" approach that is identified in the code sample.
> > >> There is greater clarity and less bytes seemingly created and written.
> > We
> > >> do sacrifice the potential ease of using an object, but in all, I
> think
> > the
> > >> way our code is structured. It is not conducive to do a value
> approach,
> > >> from an efficiency standpoint,  because of our use of the pimpl model.
> > >>>
> > >>> Thanks,
> > >>> Mark
> > >>>
> >  On Sep 12, 2017, at 11:09 AM, Jacob Barrett 
> > >> wrote:
> > 
> >  My biggest concern with this model is that access to the public
> Cache
> >  object from other public objects results in additional allocations
> of
> > a
> >  Cache value. Think about when we are inside a Serializable object
> and
> > we
> >  access the Cache from DataOutput.
> > 
> >  As value:
> >  Serializable* MyClass::fromData(DataInput& dataInput) {
> >  auto cache = dataOutput.getCache();
> >  ...
> >  }
> >  In this the value of cache will RVO the allocation of Cache in the
> > >> getCache
> >  call into the stack of this method, great. The problem is that Cache
> > >> must
> >  contain a std::shared_ptr which means that each
> allocation
> > >> is 8
> >  bytes (pointer to control block and pointer to CacheImpl) as well as
> > >> having
> >  to increment the strong counter in the control block. On
> exit/descope,
> > >> the
> >  Cache will have to decrement the control block as well.
> > 
> >  Using current shared_ptr pimple model:
> >  Serializable* MyClass::fromData(DataInput& dataInput) {
> >  auto& cache = dataOutput.getCache();
> >  ...
> >  }
> >  We only suffer the ref allocation of 4 bytes and no ref count. Since
> > >> this
> >  function call can't survive past the lifespan of Cache/CacheImpl
> they
> > >> don't
> >  need to have shared_ptr and refcounting.
> > 
> >  Given that this method could be called numerous times is the
> overhead
> > of
> >  the value version going to be a significant performance issue?
> > 
> >  I worry that moves and RVO is just beyond most developers. Heck I
> > didn't
> >  know anything about it until we started exploring it.
> > 
> > 
> > 
> >  -Jake
> > 
> > 
> >  On Tue, Sep 12, 2017 at 8:06 AM David Kimura 
> > >> wrote:
> > 
> > > Follow up of attached discussion after more 

[Spring CI] Spring Data GemFire > Nightly-ApacheGeode > #678 was SUCCESSFUL (with 2042 tests)

[Spring CI]

---
Spring Data GemFire > Nightly-ApacheGeode > #678 was successful.
---
Scheduled
2044 tests in total.

https://build.spring.io/browse/SGF-NAG-678/





--
This message is automatically generated by Atlassian Bamboo

Re: [Discuss] Investigation of C++ object return types

[Michael Martell]

Late to this party.

Confession 1: I had to look up both RVO and copy-elision.
Confession 2: I don't like using pointers at all. I used to, but I've
evolved to just use C# and Java :)

Without investing a lot more time, I don't have strong feelings about raw
vs shared pointers. My only question is: Can we return ptr to abstract
class everywhere we return objects? Just thinking of mocking, which always
wants to mock interfaces.

On Thu, Sep 14, 2017 at 2:25 PM, Michael William Dodge 
wrote:

> +0 shared pointer
>
> > On 14 Sep, 2017, at 14:09, Ernest Burghardt 
> wrote:
> >
> > Calling a vote for:
> >
> > - Raw pointer
> > - shard pointer
> >
> > +1 raw Pointer, I had to look up RVO and am new to std::move(s)
> >
> > On Thu, Sep 14, 2017 at 3:02 PM, Michael William Dodge <
> mdo...@pivotal.io>
> > wrote:
> >
> >> I generally dig reference-counted pointers for avoiding lifetime issues
> >> with objects allocated off the heap but I can live with bare pointers,
> too.
> >>
> >> Sarge
> >>
> >>> On 13 Sep, 2017, at 16:25, Mark Hanson  wrote:
> >>>
> >>> Hi All,
> >>>
> >>> I favor the “pointer" approach that is identified in the code sample.
> >> There is greater clarity and less bytes seemingly created and written.
> We
> >> do sacrifice the potential ease of using an object, but in all, I think
> the
> >> way our code is structured. It is not conducive to do a value approach,
> >> from an efficiency standpoint,  because of our use of the pimpl model.
> >>>
> >>> Thanks,
> >>> Mark
> >>>
>  On Sep 12, 2017, at 11:09 AM, Jacob Barrett 
> >> wrote:
> 
>  My biggest concern with this model is that access to the public Cache
>  object from other public objects results in additional allocations of
> a
>  Cache value. Think about when we are inside a Serializable object and
> we
>  access the Cache from DataOutput.
> 
>  As value:
>  Serializable* MyClass::fromData(DataInput& dataInput) {
>  auto cache = dataOutput.getCache();
>  ...
>  }
>  In this the value of cache will RVO the allocation of Cache in the
> >> getCache
>  call into the stack of this method, great. The problem is that Cache
> >> must
>  contain a std::shared_ptr which means that each allocation
> >> is 8
>  bytes (pointer to control block and pointer to CacheImpl) as well as
> >> having
>  to increment the strong counter in the control block. On exit/descope,
> >> the
>  Cache will have to decrement the control block as well.
> 
>  Using current shared_ptr pimple model:
>  Serializable* MyClass::fromData(DataInput& dataInput) {
>  auto& cache = dataOutput.getCache();
>  ...
>  }
>  We only suffer the ref allocation of 4 bytes and no ref count. Since
> >> this
>  function call can't survive past the lifespan of Cache/CacheImpl they
> >> don't
>  need to have shared_ptr and refcounting.
> 
>  Given that this method could be called numerous times is the overhead
> of
>  the value version going to be a significant performance issue?
> 
>  I worry that moves and RVO is just beyond most developers. Heck I
> didn't
>  know anything about it until we started exploring it.
> 
> 
> 
>  -Jake
> 
> 
>  On Tue, Sep 12, 2017 at 8:06 AM David Kimura 
> >> wrote:
> 
> > Follow up of attached discussion after more investigation.  I created
> >> an
> > example of returning Cache as shared pointer versus raw value:
> >
> >  https://github.com/dgkimura/geode-native-sandbox
> >
> > I still like returning by value as it lets the user do what they want
> >> with
> > their object.
> >
> >  // Here user creates object on their stack.
> >  auto c = CacheFactory::createFactory().create();
> >
> >  // Here user creates smart pointer in their heap.
> >  auto cptr =
> > std::make_shared(CacheFactory::createFactory().create());
> >
> > Difficulty of implementing this is high due to circular dependencies
> of
> > Cache/CacheImpl as well as objects hanging off CacheImpl that return
> > Cache.  We must be extra careful when dealing with move/copy
> semantics
> >> of
> > Cache/CacheImpl.
> >
> > Alternative, is to keep as is and only permit heap allocations from
> > factory using shared pointers.
> >
> > Thanks,
> > David
> >
> >>>
> >>
> >>
>
>

Re: [DISCUSS] Addition of isValid API to Index interface

[Anilkumar Gingade]

Dan, you are right...Thanks to Jason, myself and Jason looked into the code
to see if index is updated before the event/change is saved/injected into
the region...It looks like the index update are happening after the region
change/update is saved. Moving the index update before that is not an easy
task...

For time, when there is any problem with index update, we can proceed with
invalidating the indexes...But we really need to look at making region and
index updates in a transactional way, silently invalidating indexes may not
be acceptable...

-Anil.




On Thu, Sep 14, 2017 at 1:12 PM, Dan Smith  wrote:

> I'm still going to push that we stick with Naba's original proposal.
>
> The current behavior is clearly broken. If one index update fails, an
> exception gets thrown to the user (nice!) but it leaves the put in a
> partially completed state - some other indexes may not have been updated,
> WAN/AEQs may not have been notified, etc.
>
> We should never leave the system in this corrupted state. It would be nice
> to be able to cleanly rollback the put, but we don't have that capability
> especially considering that cache writers have already been invoked. So the
> next best thing is to invalidate the index that failed to update.
>
> Logging an error an allowing the put to succeed does match what we do with
> CacheListeners. Exceptions from CacheListeners do not fail the put.
>
> -Dan
>
> On Mon, Sep 11, 2017 at 3:29 PM, Jason Huynh  wrote:
>
> > Anil, we actually do have a case where the index is out of sync with the
> > region currently.  It's just not likely to happen but if there is an
> > exception from an index, the end result is that certain indexes get
> updated
> > and the region has already been updated.
> > However the exception is thrown back to the putter, so it becomes very
> > obvious something is wrong but I believe Naba has updated the ticket to
> > show a test that reproduces the problem...
> >
> >
> > On Mon, Sep 11, 2017 at 2:50 PM Anilkumar Gingade 
> > wrote:
> >
> > > The other way to look at it is; what happens to a cache op; when there
> is
> > > an exception after Region.Entry is created? can it happen? In that
> case,
> > do
> > > we stick the entry into the Cache or not? If an exception is handled,
> how
> > > is it done, can we look at using the same for Index...
> > >
> > > Also previously, once the valid index is created (verified during
> create
> > or
> > > first put into the cache); we never had any issue where index is out of
> > > sync with cache...If that changes with new futures (security?) then we
> > may
> > > have to change the expectation with indexing...
> > >
> > > -Anil.
> > >
> > >
> > >
> > > On Mon, Sep 11, 2017 at 2:16 PM, Anthony Baker 
> > wrote:
> > >
> > > > I’m confused.  Once a cache update has been distributed to other
> > members
> > > > it can’t be undone.  That update could have triggered myriad other
> > > > application behaviors.
> > > >
> > > > Anthony
> > > >
> > > > > On Sep 11, 2017, at 2:04 PM, Michael Stolz 
> > wrote:
> > > > >
> > > > > Great, that's exactly the behavior I would expect.
> > > > >
> > > > > Thanks.
> > > > >
> > > > > --
> > > > > Mike Stolz
> > > > > Principal Engineer, GemFire Product Manager
> > > > > Mobile: +1-631-835-4771 <(631)%20835-4771>
> > > > >
> > > > > On Mon, Sep 11, 2017 at 4:34 PM, Jason Huynh 
> > > wrote:
> > > > >
> > > > >> Hi Mike, I think the concern was less about the security portion
> but
> > > > rather
> > > > >> if any exception occurs during index update, right now, the region
> > > gets
> > > > >> updated and the rest of the system (index/wan/callbacks) may or
> may
> > > not
> > > > be
> > > > >> updated.  I think Naba just tried to provide an example where this
> > > might
> > > > >> occur, but that specific scenario is invalid.
> > > > >>
> > > > >> I believe Nabarun has opened a ticket for rolling back the put
> > > operation
> > > > >> when an index exception occurs. GEODE-3589.  It can probably be
> > > > modified to
> > > > >> state any exception instead of index exceptions.
> > > > >>
> > > > >> To summarize my understanding:
> > > > >> -Someone will need to implement the rollback for GEODE-3589.  This
> > > means
> > > > >> that if any exception occurs during a put, geode it will propagate
> > > back
> > > > to
> > > > >> the user and it is expected the rollback mechanism will clean up
> any
> > > > >> partial put.
> > > > >>
> > > > >> GEODE-3520 should be modified to:
> > > > >> -Add the isValid() api to index interface
> > > > >> -Mark an index as invalid during async index updates but not for
> > > > >> synchronous index updates.  The synchronous index updates will
> rely
> > > on a
> > > > >> rollback mechanism
> > > > >>
> > > > >>
> > > > >>
> > > > >>
> > > > >> On Mon, Sep 11, 2017 at 1:23 PM Michael Stolz 
> > > > wrote:
> > > > >>
> > > 

Re: [Discuss] Investigation of C++ object return types

[Michael William Dodge]

+0 shared pointer

> On 14 Sep, 2017, at 14:09, Ernest Burghardt  wrote:
> 
> Calling a vote for:
> 
> - Raw pointer
> - shard pointer
> 
> +1 raw Pointer, I had to look up RVO and am new to std::move(s)
> 
> On Thu, Sep 14, 2017 at 3:02 PM, Michael William Dodge 
> wrote:
> 
>> I generally dig reference-counted pointers for avoiding lifetime issues
>> with objects allocated off the heap but I can live with bare pointers, too.
>> 
>> Sarge
>> 
>>> On 13 Sep, 2017, at 16:25, Mark Hanson  wrote:
>>> 
>>> Hi All,
>>> 
>>> I favor the “pointer" approach that is identified in the code sample.
>> There is greater clarity and less bytes seemingly created and written. We
>> do sacrifice the potential ease of using an object, but in all, I think the
>> way our code is structured. It is not conducive to do a value approach,
>> from an efficiency standpoint,  because of our use of the pimpl model.
>>> 
>>> Thanks,
>>> Mark
>>> 
 On Sep 12, 2017, at 11:09 AM, Jacob Barrett 
>> wrote:
 
 My biggest concern with this model is that access to the public Cache
 object from other public objects results in additional allocations of a
 Cache value. Think about when we are inside a Serializable object and we
 access the Cache from DataOutput.
 
 As value:
 Serializable* MyClass::fromData(DataInput& dataInput) {
 auto cache = dataOutput.getCache();
 ...
 }
 In this the value of cache will RVO the allocation of Cache in the
>> getCache
 call into the stack of this method, great. The problem is that Cache
>> must
 contain a std::shared_ptr which means that each allocation
>> is 8
 bytes (pointer to control block and pointer to CacheImpl) as well as
>> having
 to increment the strong counter in the control block. On exit/descope,
>> the
 Cache will have to decrement the control block as well.
 
 Using current shared_ptr pimple model:
 Serializable* MyClass::fromData(DataInput& dataInput) {
 auto& cache = dataOutput.getCache();
 ...
 }
 We only suffer the ref allocation of 4 bytes and no ref count. Since
>> this
 function call can't survive past the lifespan of Cache/CacheImpl they
>> don't
 need to have shared_ptr and refcounting.
 
 Given that this method could be called numerous times is the overhead of
 the value version going to be a significant performance issue?
 
 I worry that moves and RVO is just beyond most developers. Heck I didn't
 know anything about it until we started exploring it.
 
 
 
 -Jake
 
 
 On Tue, Sep 12, 2017 at 8:06 AM David Kimura 
>> wrote:
 
> Follow up of attached discussion after more investigation.  I created
>> an
> example of returning Cache as shared pointer versus raw value:
> 
>  https://github.com/dgkimura/geode-native-sandbox
> 
> I still like returning by value as it lets the user do what they want
>> with
> their object.
> 
>  // Here user creates object on their stack.
>  auto c = CacheFactory::createFactory().create();
> 
>  // Here user creates smart pointer in their heap.
>  auto cptr =
> std::make_shared(CacheFactory::createFactory().create());
> 
> Difficulty of implementing this is high due to circular dependencies of
> Cache/CacheImpl as well as objects hanging off CacheImpl that return
> Cache.  We must be extra careful when dealing with move/copy semantics
>> of
> Cache/CacheImpl.
> 
> Alternative, is to keep as is and only permit heap allocations from
> factory using shared pointers.
> 
> Thanks,
> David
> 
>>> 
>> 
>> 

Re: [DISCUSS] geode-native c++ exceptions

[Michael William Dodge]

+1 for avoiding multiple inheritance

> On 14 Sep, 2017, at 14:23, Ernest Burghardt  wrote:
> 
> Sounds like the proposal currently stands to avoid the DiamondOfDeath or
> TriangleOfLove that multiple inheritance brings us
> and just have the base Exception class inherit std::exception and extend
> Exception class as appropriate within the library.
> 
> +1  and thanks for the code examples, very illistrative!
> 
> +1 to Boost stack trace as well...
> 
> 
> On Thu, Sep 14, 2017 at 10:10 AM, Jacob Barrett  wrote:
> 
>> The problem stems from the fact that none of the std exceptions virtually
>> inherit from std::exception so you end up in the inheritance triangle of
>> love. I say we avoid the multiple inheritance issues with exceptions by
>> avoiding multiple inheritance altogether in exceptions. See this example.
>> http://coliru.stacked-crooked.com/a/2e32eb021e85
>> 
>> Basically all of our exceptions extend our Exception class which extends
>> std::exception. None extend any of the other std exceptions, like
>> bad_alloc, etc. The downside is you can't catch any std exception other
>> than std::exception but the upside is that this actually works. There were
>> also very few exceptions that actually could extend any std exceptions
>> beyond std::exception.
>> 
>> -Jake
>> 
>> 
>> On Wed, Sep 13, 2017 at 10:52 PM Jacob Barrett 
>> wrote:
>> 
>>> Let me see if I can considerate the consensus here:
>>> 
>>> As reasonably possible throw library specific exceptions from all public
>>> methods.
>>> 
>>> As reasonably possible those exceptions should extend C++11 standard
>>> library exceptions.
>>> 
>>> All exceptions should extend less specific but logically relevant base
>>> exceptions.
>>> 
>>> A few quick examples:
>>> namespace apache {
>>> namespace geode {
>>> namespace client {
>>> 
>>> class Exception : public std::exception {...};
>>> 
>>> class IllegalArgumentException : public Exception, public
>>> std::invalid_argument {...};
>>> 
>>> class TransactionException : public Exception {...};
>>> 
>>> class RollbackException : public TransactionException {...};
>>> 
>>> }
>>> }
>>> }
>>> 
>>> Additionally, investigate using Boost Stack Track library for providing
>>> stack context in exceptions, otherwise dump the current stack tracing
>>> feature that is incomplete and very platform specific.
>>> 
>>> Does anyone have a different understanding of the consensus?
>>> 
>>> 
>>> I found some problems with this model. The IllegalArgumentException would
>>> not be caught in a catch (const std::exception& e) statement do to
>>> multiple inheritance. Another nasty is std::exception doesn't have a
>>> constructor to populate the what() value. Some examples of this problem
>>> can be seen here:
>>> http://coliru.stacked-crooked.com/a/5b6e34c7ea020fc8
>>> 
>>> 
>>> -Jake
>>> 
>>> 
>>> 
>>> 
>>> On Wed, Sep 13, 2017 at 4:37 PM Mark Hanson  wrote:
>>> 
 I think that it would be best to abide by using the std::exception as
>> the
 base. I think it brings with it all the language support and
>> flexibility.
 There are a few penalties obviously to using std::exception as a base,
>> but
 I think they are sufficiently offset by using a standard. As far as the
 number of exceptions, I believe in the philosophy of using as few as
 possible and using inheritance to drive specificity. The benefit is that
 the code can be as simple or as complex as it can be without unnecessary
 overhead e.g error => network error => tcp error. So, I may just care
>> there
 is a network error and the being able to tell if something derives from
 network error is perfect.
 
 Thanks,
 Mark
 
 
> On Sep 8, 2017, at 1:35 PM, Ernest Burghardt 
 wrote:
> 
> if we continue the merging of Jake <> Sarge comments we might find
>> that
> std::exception(s) is sufficient if the many name exceptions that
 pertain to
> the Library are all handled in a similar manner and only have unique
 names
> for human readability, but not a functional consideration...
> 
> EB
> 
> On Fri, Sep 8, 2017 at 8:52 AM, Michael William Dodge <
 mdo...@pivotal.io>
> wrote:
> 
>> I subscribe to Josh Gray's philosophy of only having another
>> exception
>> class if there is something different to be done when it's caught.
>> For
>> example, if the caller would do the exact same thing for
>> NoPermissionsException and DiskFullException, just use an IOException
 and
>> be done with it. I also subscribe to the philosophy that a library
 have its
>> own exception hierarchy (possibly with a single class), which I think
>> meshes with Jake's "exceptions exiting a library to be reasonably
 limited
>> to exceptions generated by and relating to that library".
>> 
>> Sarge
>> 
>>> On 8 Sep, 

Re: [DISCUSS] geode-native c++ exceptions

[Ernest Burghardt]

Sounds like the proposal currently stands to avoid the DiamondOfDeath or
TriangleOfLove that multiple inheritance brings us
and just have the base Exception class inherit std::exception and extend
Exception class as appropriate within the library.

+1  and thanks for the code examples, very illistrative!

+1 to Boost stack trace as well...


On Thu, Sep 14, 2017 at 10:10 AM, Jacob Barrett  wrote:

> The problem stems from the fact that none of the std exceptions virtually
> inherit from std::exception so you end up in the inheritance triangle of
> love. I say we avoid the multiple inheritance issues with exceptions by
> avoiding multiple inheritance altogether in exceptions. See this example.
> http://coliru.stacked-crooked.com/a/2e32eb021e85
>
> Basically all of our exceptions extend our Exception class which extends
> std::exception. None extend any of the other std exceptions, like
> bad_alloc, etc. The downside is you can't catch any std exception other
> than std::exception but the upside is that this actually works. There were
> also very few exceptions that actually could extend any std exceptions
> beyond std::exception.
>
> -Jake
>
>
> On Wed, Sep 13, 2017 at 10:52 PM Jacob Barrett 
> wrote:
>
> > Let me see if I can considerate the consensus here:
> >
> > As reasonably possible throw library specific exceptions from all public
> > methods.
> >
> > As reasonably possible those exceptions should extend C++11 standard
> > library exceptions.
> >
> > All exceptions should extend less specific but logically relevant base
> > exceptions.
> >
> > A few quick examples:
> > namespace apache {
> > namespace geode {
> > namespace client {
> >
> > class Exception : public std::exception {...};
> >
> > class IllegalArgumentException : public Exception, public
> > std::invalid_argument {...};
> >
> > class TransactionException : public Exception {...};
> >
> > class RollbackException : public TransactionException {...};
> >
> > }
> > }
> > }
> >
> > Additionally, investigate using Boost Stack Track library for providing
> > stack context in exceptions, otherwise dump the current stack tracing
> > feature that is incomplete and very platform specific.
> >
> > Does anyone have a different understanding of the consensus?
> >
> >
> > I found some problems with this model. The IllegalArgumentException would
> > not be caught in a catch (const std::exception& e) statement do to
> > multiple inheritance. Another nasty is std::exception doesn't have a
> > constructor to populate the what() value. Some examples of this problem
> > can be seen here:
> > http://coliru.stacked-crooked.com/a/5b6e34c7ea020fc8
> >
> >
> > -Jake
> >
> >
> >
> >
> > On Wed, Sep 13, 2017 at 4:37 PM Mark Hanson  wrote:
> >
> >> I think that it would be best to abide by using the std::exception as
> the
> >> base. I think it brings with it all the language support and
> flexibility.
> >> There are a few penalties obviously to using std::exception as a base,
> but
> >> I think they are sufficiently offset by using a standard. As far as the
> >> number of exceptions, I believe in the philosophy of using as few as
> >> possible and using inheritance to drive specificity. The benefit is that
> >> the code can be as simple or as complex as it can be without unnecessary
> >> overhead e.g error => network error => tcp error. So, I may just care
> there
> >> is a network error and the being able to tell if something derives from
> >> network error is perfect.
> >>
> >> Thanks,
> >> Mark
> >>
> >>
> >> > On Sep 8, 2017, at 1:35 PM, Ernest Burghardt 
> >> wrote:
> >> >
> >> > if we continue the merging of Jake <> Sarge comments we might find
> that
> >> > std::exception(s) is sufficient if the many name exceptions that
> >> pertain to
> >> > the Library are all handled in a similar manner and only have unique
> >> names
> >> > for human readability, but not a functional consideration...
> >> >
> >> > EB
> >> >
> >> > On Fri, Sep 8, 2017 at 8:52 AM, Michael William Dodge <
> >> mdo...@pivotal.io>
> >> > wrote:
> >> >
> >> >> I subscribe to Josh Gray's philosophy of only having another
> exception
> >> >> class if there is something different to be done when it's caught.
> For
> >> >> example, if the caller would do the exact same thing for
> >> >> NoPermissionsException and DiskFullException, just use an IOException
> >> and
> >> >> be done with it. I also subscribe to the philosophy that a library
> >> have its
> >> >> own exception hierarchy (possibly with a single class), which I think
> >> >> meshes with Jake's "exceptions exiting a library to be reasonably
> >> limited
> >> >> to exceptions generated by and relating to that library".
> >> >>
> >> >> Sarge
> >> >>
> >> >>> On 8 Sep, 2017, at 07:19, Jacob Barrett 
> wrote:
> >> >>>
> >> >>> Sorry for jumping on this thread so late. This is an important issue
> >> we
> >> >>> need to address.

Re: [Discuss] Investigation of C++ object return types

[Ernest Burghardt]

Calling a vote for:

- Raw pointer
- shard pointer

+1 raw Pointer, I had to look up RVO and am new to std::move(s)

On Thu, Sep 14, 2017 at 3:02 PM, Michael William Dodge 
wrote:

> I generally dig reference-counted pointers for avoiding lifetime issues
> with objects allocated off the heap but I can live with bare pointers, too.
>
> Sarge
>
> > On 13 Sep, 2017, at 16:25, Mark Hanson  wrote:
> >
> > Hi All,
> >
> > I favor the “pointer" approach that is identified in the code sample.
> There is greater clarity and less bytes seemingly created and written. We
> do sacrifice the potential ease of using an object, but in all, I think the
> way our code is structured. It is not conducive to do a value approach,
> from an efficiency standpoint,  because of our use of the pimpl model.
> >
> > Thanks,
> > Mark
> >
> >> On Sep 12, 2017, at 11:09 AM, Jacob Barrett 
> wrote:
> >>
> >> My biggest concern with this model is that access to the public Cache
> >> object from other public objects results in additional allocations of a
> >> Cache value. Think about when we are inside a Serializable object and we
> >> access the Cache from DataOutput.
> >>
> >> As value:
> >> Serializable* MyClass::fromData(DataInput& dataInput) {
> >> auto cache = dataOutput.getCache();
> >> ...
> >> }
> >> In this the value of cache will RVO the allocation of Cache in the
> getCache
> >> call into the stack of this method, great. The problem is that Cache
> must
> >> contain a std::shared_ptr which means that each allocation
> is 8
> >> bytes (pointer to control block and pointer to CacheImpl) as well as
> having
> >> to increment the strong counter in the control block. On exit/descope,
> the
> >> Cache will have to decrement the control block as well.
> >>
> >> Using current shared_ptr pimple model:
> >> Serializable* MyClass::fromData(DataInput& dataInput) {
> >> auto& cache = dataOutput.getCache();
> >> ...
> >> }
> >> We only suffer the ref allocation of 4 bytes and no ref count. Since
> this
> >> function call can't survive past the lifespan of Cache/CacheImpl they
> don't
> >> need to have shared_ptr and refcounting.
> >>
> >> Given that this method could be called numerous times is the overhead of
> >> the value version going to be a significant performance issue?
> >>
> >> I worry that moves and RVO is just beyond most developers. Heck I didn't
> >> know anything about it until we started exploring it.
> >>
> >>
> >>
> >> -Jake
> >>
> >>
> >> On Tue, Sep 12, 2017 at 8:06 AM David Kimura 
> wrote:
> >>
> >>> Follow up of attached discussion after more investigation.  I created
> an
> >>> example of returning Cache as shared pointer versus raw value:
> >>>
> >>>   https://github.com/dgkimura/geode-native-sandbox
> >>>
> >>> I still like returning by value as it lets the user do what they want
> with
> >>> their object.
> >>>
> >>>   // Here user creates object on their stack.
> >>>   auto c = CacheFactory::createFactory().create();
> >>>
> >>>   // Here user creates smart pointer in their heap.
> >>>   auto cptr =
> >>> std::make_shared(CacheFactory::createFactory().create());
> >>>
> >>> Difficulty of implementing this is high due to circular dependencies of
> >>> Cache/CacheImpl as well as objects hanging off CacheImpl that return
> >>> Cache.  We must be extra careful when dealing with move/copy semantics
> of
> >>> Cache/CacheImpl.
> >>>
> >>> Alternative, is to keep as is and only permit heap allocations from
> >>> factory using shared pointers.
> >>>
> >>> Thanks,
> >>> David
> >>>
> >
>
>

Re: [DISCUSS] Change signature of Serializable::fromData on Geode-Native

[Michael William Dodge]

+1 for the void return type

> On 14 Sep, 2017, at 13:39, Ernest Burghardt  wrote:
> 
> +1  cleans up the public API and code using this as you can see in the
> proposed changes on Jake's fork
> 
> On Wed, Sep 13, 2017 at 3:30 PM, Jacob Barrett  wrote:
> 
>> I would like to propose a change:
>> Serializable* Serializable::formData(DataInput& in)
>> to
>> void Serializable::formData(DataInput& in)
>> 
>> The current signature allows the object being deserialized to return an
>> object other than itself. The use of this trick is only done internally for
>> a few internal system types in what appears to have been an attempt to make
>> serialization more pluggable. The downside to this approach is that it
>> leaks this ability out to the public interface. Additionally concerning is
>> that the return type is a raw pointer to Serializable but typically the
>> object was created as a std::shared_ptr, which can lead to shard_ptr errors
>> if you don't property check and swap the returned raw pointer against the
>> original shared_ptr. Lastly the return value is a pointer to the most base
>> interface providing little utility and type safety.
>> 
>> A couple of spikes investigated changing the signature to:
>> std::shared_ptr Serializable::formData(DataInput& in)
>> and:
>> template
>> std::shared_ptr Serializable::formData(DataInput& in)
>> But both approaches left some dirty things laying around. First the
>> templated version just caused all sorts of pain and failed when the value
>> was replaced on the fromData. The more generic share_ptr
>> approach uncovered a plethora of places internally that we use the
>> Serializable::fromData to deserialize objects as parts of protocol messages
>> and used as internal data where they weren't originally created as
>> shared_ptrs, so the opposite problem to what we were trying to solve.
>> 
>> The final spike investigated using void. In doing so we only had to make
>> small changes to the way PDX types were being deserialized. The void
>> signature is also more consistent with the Java DataSerializable interface.
>> By making it void the ambiguity of using or checking the return value goes
>> away.
>> 
>> You can see the proposed changes on my fork at
>> https://github.com/pivotal-jbarrett/geode-native/tree/wip/fromDataVoid.
>> 
>> Thoughts?
>> 
>> -Jake
>> 

Re: [Discuss] Investigation of C++ object return types

[Michael William Dodge]

I generally dig reference-counted pointers for avoiding lifetime issues with 
objects allocated off the heap but I can live with bare pointers, too.

Sarge

> On 13 Sep, 2017, at 16:25, Mark Hanson  wrote:
> 
> Hi All,
> 
> I favor the “pointer" approach that is identified in the code sample. There 
> is greater clarity and less bytes seemingly created and written. We do 
> sacrifice the potential ease of using an object, but in all, I think the way 
> our code is structured. It is not conducive to do a value approach,  from an 
> efficiency standpoint,  because of our use of the pimpl model. 
> 
> Thanks,
> Mark
> 
>> On Sep 12, 2017, at 11:09 AM, Jacob Barrett  wrote:
>> 
>> My biggest concern with this model is that access to the public Cache
>> object from other public objects results in additional allocations of a
>> Cache value. Think about when we are inside a Serializable object and we
>> access the Cache from DataOutput.
>> 
>> As value:
>> Serializable* MyClass::fromData(DataInput& dataInput) {
>> auto cache = dataOutput.getCache();
>> ...
>> }
>> In this the value of cache will RVO the allocation of Cache in the getCache
>> call into the stack of this method, great. The problem is that Cache must
>> contain a std::shared_ptr which means that each allocation is 8
>> bytes (pointer to control block and pointer to CacheImpl) as well as having
>> to increment the strong counter in the control block. On exit/descope, the
>> Cache will have to decrement the control block as well.
>> 
>> Using current shared_ptr pimple model:
>> Serializable* MyClass::fromData(DataInput& dataInput) {
>> auto& cache = dataOutput.getCache();
>> ...
>> }
>> We only suffer the ref allocation of 4 bytes and no ref count. Since this
>> function call can't survive past the lifespan of Cache/CacheImpl they don't
>> need to have shared_ptr and refcounting.
>> 
>> Given that this method could be called numerous times is the overhead of
>> the value version going to be a significant performance issue?
>> 
>> I worry that moves and RVO is just beyond most developers. Heck I didn't
>> know anything about it until we started exploring it.
>> 
>> 
>> 
>> -Jake
>> 
>> 
>> On Tue, Sep 12, 2017 at 8:06 AM David Kimura  wrote:
>> 
>>> Follow up of attached discussion after more investigation.  I created an
>>> example of returning Cache as shared pointer versus raw value:
>>> 
>>>   https://github.com/dgkimura/geode-native-sandbox
>>> 
>>> I still like returning by value as it lets the user do what they want with
>>> their object.
>>> 
>>>   // Here user creates object on their stack.
>>>   auto c = CacheFactory::createFactory().create();
>>> 
>>>   // Here user creates smart pointer in their heap.
>>>   auto cptr =
>>> std::make_shared(CacheFactory::createFactory().create());
>>> 
>>> Difficulty of implementing this is high due to circular dependencies of
>>> Cache/CacheImpl as well as objects hanging off CacheImpl that return
>>> Cache.  We must be extra careful when dealing with move/copy semantics of
>>> Cache/CacheImpl.
>>> 
>>> Alternative, is to keep as is and only permit heap allocations from
>>> factory using shared pointers.
>>> 
>>> Thanks,
>>> David
>>> 
> 

Re: [DISCUSS] Change signature of Serializable::fromData on Geode-Native

[Ernest Burghardt]

+1  cleans up the public API and code using this as you can see in the
proposed changes on Jake's fork

On Wed, Sep 13, 2017 at 3:30 PM, Jacob Barrett  wrote:

> I would like to propose a change:
> Serializable* Serializable::formData(DataInput& in)
> to
> void Serializable::formData(DataInput& in)
>
> The current signature allows the object being deserialized to return an
> object other than itself. The use of this trick is only done internally for
> a few internal system types in what appears to have been an attempt to make
> serialization more pluggable. The downside to this approach is that it
> leaks this ability out to the public interface. Additionally concerning is
> that the return type is a raw pointer to Serializable but typically the
> object was created as a std::shared_ptr, which can lead to shard_ptr errors
> if you don't property check and swap the returned raw pointer against the
> original shared_ptr. Lastly the return value is a pointer to the most base
> interface providing little utility and type safety.
>
> A couple of spikes investigated changing the signature to:
> std::shared_ptr Serializable::formData(DataInput& in)
> and:
> template
> std::shared_ptr Serializable::formData(DataInput& in)
> But both approaches left some dirty things laying around. First the
> templated version just caused all sorts of pain and failed when the value
> was replaced on the fromData. The more generic share_ptr
> approach uncovered a plethora of places internally that we use the
> Serializable::fromData to deserialize objects as parts of protocol messages
> and used as internal data where they weren't originally created as
> shared_ptrs, so the opposite problem to what we were trying to solve.
>
> The final spike investigated using void. In doing so we only had to make
> small changes to the way PDX types were being deserialized. The void
> signature is also more consistent with the Java DataSerializable interface.
> By making it void the ambiguity of using or checking the return value goes
> away.
>
> You can see the proposed changes on my fork at
> https://github.com/pivotal-jbarrett/geode-native/tree/wip/fromDataVoid.
>
> Thoughts?
>
> -Jake
>

Re: [DISCUSS] Addition of isValid API to Index interface

[Dan Smith]

I'm still going to push that we stick with Naba's original proposal.

The current behavior is clearly broken. If one index update fails, an
exception gets thrown to the user (nice!) but it leaves the put in a
partially completed state - some other indexes may not have been updated,
WAN/AEQs may not have been notified, etc.

We should never leave the system in this corrupted state. It would be nice
to be able to cleanly rollback the put, but we don't have that capability
especially considering that cache writers have already been invoked. So the
next best thing is to invalidate the index that failed to update.

Logging an error an allowing the put to succeed does match what we do with
CacheListeners. Exceptions from CacheListeners do not fail the put.

-Dan

On Mon, Sep 11, 2017 at 3:29 PM, Jason Huynh  wrote:

> Anil, we actually do have a case where the index is out of sync with the
> region currently.  It's just not likely to happen but if there is an
> exception from an index, the end result is that certain indexes get updated
> and the region has already been updated.
> However the exception is thrown back to the putter, so it becomes very
> obvious something is wrong but I believe Naba has updated the ticket to
> show a test that reproduces the problem...
>
>
> On Mon, Sep 11, 2017 at 2:50 PM Anilkumar Gingade 
> wrote:
>
> > The other way to look at it is; what happens to a cache op; when there is
> > an exception after Region.Entry is created? can it happen? In that case,
> do
> > we stick the entry into the Cache or not? If an exception is handled, how
> > is it done, can we look at using the same for Index...
> >
> > Also previously, once the valid index is created (verified during create
> or
> > first put into the cache); we never had any issue where index is out of
> > sync with cache...If that changes with new futures (security?) then we
> may
> > have to change the expectation with indexing...
> >
> > -Anil.
> >
> >
> >
> > On Mon, Sep 11, 2017 at 2:16 PM, Anthony Baker 
> wrote:
> >
> > > I’m confused.  Once a cache update has been distributed to other
> members
> > > it can’t be undone.  That update could have triggered myriad other
> > > application behaviors.
> > >
> > > Anthony
> > >
> > > > On Sep 11, 2017, at 2:04 PM, Michael Stolz 
> wrote:
> > > >
> > > > Great, that's exactly the behavior I would expect.
> > > >
> > > > Thanks.
> > > >
> > > > --
> > > > Mike Stolz
> > > > Principal Engineer, GemFire Product Manager
> > > > Mobile: +1-631-835-4771 <(631)%20835-4771>
> > > >
> > > > On Mon, Sep 11, 2017 at 4:34 PM, Jason Huynh 
> > wrote:
> > > >
> > > >> Hi Mike, I think the concern was less about the security portion but
> > > rather
> > > >> if any exception occurs during index update, right now, the region
> > gets
> > > >> updated and the rest of the system (index/wan/callbacks) may or may
> > not
> > > be
> > > >> updated.  I think Naba just tried to provide an example where this
> > might
> > > >> occur, but that specific scenario is invalid.
> > > >>
> > > >> I believe Nabarun has opened a ticket for rolling back the put
> > operation
> > > >> when an index exception occurs. GEODE-3589.  It can probably be
> > > modified to
> > > >> state any exception instead of index exceptions.
> > > >>
> > > >> To summarize my understanding:
> > > >> -Someone will need to implement the rollback for GEODE-3589.  This
> > means
> > > >> that if any exception occurs during a put, geode it will propagate
> > back
> > > to
> > > >> the user and it is expected the rollback mechanism will clean up any
> > > >> partial put.
> > > >>
> > > >> GEODE-3520 should be modified to:
> > > >> -Add the isValid() api to index interface
> > > >> -Mark an index as invalid during async index updates but not for
> > > >> synchronous index updates.  The synchronous index updates will rely
> > on a
> > > >> rollback mechanism
> > > >>
> > > >>
> > > >>
> > > >>
> > > >> On Mon, Sep 11, 2017 at 1:23 PM Michael Stolz 
> > > wrote:
> > > >>
> > > >>> I think there was an intention of having CREATION of an index
> > require a
> > > >>> higher privilege than DATA:WRITE, but it shouldn't affect applying
> > the
> > > >>> index on either of put or get operations.
> > > >>>
> > > >>> If we are requiring something like CLUSTER:MANAGE for put on an
> > indexed
> > > >>> region, that is an incorrect requirement. Only DATA:WRITE should be
> > > >>> required to put an entry and have it be indexed if an index is
> > present.
> > > >>>
> > > >>> --
> > > >>> Mike Stolz
> > > >>> Principal Engineer, GemFire Product Manager
> > > >>> Mobile: +1-631-835-4771 <(631)%20835-4771> <(631)%20835-4771>
> > > >>>
> > > >>> On Fri, Sep 8, 2017 at 6:04 PM, Anilkumar Gingade <
> > aging...@pivotal.io
> > > >
> > > >>> wrote:
> > > >>>
> > >  Indexes are critical for querying; most of the databases doesn't
> > allow
> > > 

Re: [Discuss] Moving away from virtual CacheableStringPtr toString() for Serializable objects in debug and logging to std::string and std::wstring

[Jacob Barrett]

Don't use std::wstring EVER. The std::wstring has a platform dependent
storage class. This means on Windows it is 32bits and Linux it is 16bits.
None of the std::string types, and there are more than std::wstring, define
an encoding, so would the wstring be UTF-16, UC-2, UTF-32, UC-4, or some
other local non-unicode codepage. It gets nasty fast!

If you only support std::string and require the all std::string's be
encoded with UTF-8 then things get real clear really fast. Yes there is
some overhead in converting to UC-4 for calling windows system calls that
want Unicode 32-bit std::wstring but we already had that overhead since
almost all strings in NC were actually 8-bit UTF-8 std::string. And since
almost all those are ASCII strings and UTF-8 can encode ASCII without
overhead we are good to go!

So to simply we would change Serializable::toString to only a std::string.

class Serializable {
...
virtual const std::string& toString() const noexcept;
}

So the example use would be:

auto myObject = region.get("myObject"); // auto = std::shared_ptr
auto myString = myObject->toString(); // auto = std::string
std::cout << myString;

Even nicer may be to implement a default << operator for Serializable the
outputs the toString so we can do:

std::cout << region.get("myObject");

As for exporting a to_string into the std namespace, I think that is one
that is actually forbidden unlike std::hash/equal_to. I feel like I found
that when trying to output std::chrono::duration objects to stream. Can you
double check that it is allowed?

-Jake



On Thu, Sep 14, 2017 at 11:30 AM David Kimura  wrote:

> Seems like a good idea.
>
> Here's a quick thought - I think c++11 is not really an obj.toString() kind
> of language (like Java or C#).  Would it make sense to add std::to_string()
> and std::to_wstring() equivalents for CacheableString?  This might mean
> implementing following functions:
>
> std::string to_string(CacheableString value);
> std::wstring to_wstring(CacheableString value);
>
> So that we can do something like:
>
> std::cout << std::to_wstring(pdxser);
>
> Thanks,
> David
>
> On Thu, Sep 14, 2017 at 11:10 AM, Michael William Dodge  >
> wrote:
>
> > +1 for std::string and std::wstring.
> >
> > Sarge
> >
> > > On 14 Sep, 2017, at 11:10, Mark Hanson  wrote:
> > >
> > > Hi All,
> > >
> > > I wanted to broach the subject of moving away from moving away from
> > CacheableStringPtrs for the toString representation of Serializable. It
> > would seem desirable to move to std::string and std::wstring to use more
> > basic types that would be faster to log and the code would be simpler
> for a
> > user.
> > >
> > > Are there any opinions on this subject?
> > >
> > > Here is a before and after look at a chunk of code
> > >
> > > Before
> > >
> > > CacheableStringPtr ptr = pdxser->toString();
> > > if (ptr->isWideString()) {
> > >  printf(" query idx %d pulled object %S  :: \n", i,
> > > ptr->asWChar());
> > > } else {
> > >  printf(" query idx %d pulled object %s  :: \n", i,
> > > ptr->asChar());
> > > }
> > >
> > > After
> > >
> > >
> > > if (pdxser->isWideString()) {
> > >   std::cout << " query idx “ << i << "pulled object ” <<
> > pdxser->toWString() << std::endl;
> > > } else {
> > >   std::cout << " query idx “ << i << "pulled object ” <<
> > pdxser->toString() << std::endl;
> > > }
> > >
> > >
> > > Thanks,
> > > Mark
> >
> >
>

Re: [Discuss] Moving away from virtual CacheableStringPtr toString() for Serializable objects in debug and logging to std::string and std::wstring

[Jacob Barrett]

On Thu, Sep 14, 2017 at 11:10 AM Mark Hanson  wrote:

> if (pdxser->isWideString()) {
>std::cout << " query idx “ << i << "pulled object ” <<
> pdxser->toWString() << std::endl;
> } else {
>std::cout << " query idx “ << i << "pulled object ” <<
> pdxser->toString() << std::endl;
> }
>

How does your pdxser object have an "isWideString()" method?

Re: Review Request 62339: Will this preserve backwards compat?

[Patrick Rhomberg]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62339/#review185442
---




geode-core/src/main/java/org/apache/geode/security/ResourcePermission.java
Lines 78-91 (patched)


We had discussed moving towards a strictly-String API in any case.  I 
didn't know if this should be marked `@Deprecated` or not as a result.


- Patrick Rhomberg


On Sept. 14, 2017, 7:05 p.m., Patrick Rhomberg wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62339/
> ---
> 
> (Updated Sept. 14, 2017, 7:05 p.m.)
> 
> 
> Review request for geode.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> Will this preserve backwards compat?
> 
> 
> Diffs
> -
> 
>   geode-core/src/main/java/org/apache/geode/security/ResourcePermission.java 
> 6a920d7c217f7383e38a7465540cdc0d542b6c81 
> 
> 
> Diff: https://reviews.apache.org/r/62339/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Patrick Rhomberg
> 
>

Re: [DISCUSS] GEODE-3617 Replace gemfire prefix with geode

[Bruce Schuchardt]

There are plenty of places besides DistributionConfig that use "gemfire" 
prefixed system properties and there are now also places that use 
"geode" prefixed system properties.  I think the whole mess needs to be 
managed and allow either prefix, or even as someone suggested making it 
plug-able.  There is also an HTML file that, now out of date, lists most 
of these system properties.



On 9/14/17 10:14 AM, Darrel Schneider wrote:

+1 to having DistributionConfig look for both the "gemfire." and "geode."
prefixes.
+1 to having DistributionConfig look for both a "gemfire.properties" and
"geode.properties" file.
Since the geode flavors are newer it should look for them first and only
look for the old gemfire flavor if a geode one is not found.

On Thu, Sep 14, 2017 at 9:35 AM, Kirk Lund  wrote:


That's a bigger change and I'm not sure how you would handle backwards
compatibility for users using gemfire.properties.

On Thu, Sep 14, 2017 at 9:15 AM, Jacob Barrett 
wrote:


Or better yet, we stop using properties files already.

On Thu, Sep 14, 2017 at 8:55 AM Dave Barnes  wrote:


Is there a possibility that the code might find its way into additional
contexts with other names? If so, perhaps we should consider a more

generic

identifier, such as PRODUCT_PREFIX.

On Thu, Sep 14, 2017 at 4:42 AM, Dinesh Akhand 
wrote:


Hi,

Why we are keeping gemfire in current geode 1.2 , Can we replace this

with

GEODE
File : DistributionConfig.java

Current code:
   String GEMFIRE_PREFIX = "gemfire.";

Suggestion to change:
  String GEODE_PREFIX = "geode.";

Why do you think ?
Can we go ahead  and change this ?
It will impact lots of files & all configuration will be now using

with

geode.

Thanks,
Dinesh Akhand
This message and the information contained herein is proprietary and
confidential and subject to the Amdocs policy statement,

you may review at https://www.amdocs.com/about/email-disclaimer <
https://www.amdocs.com/about/email-disclaimer>

Review Request 62339: Will this preserve backwards compat?

[Patrick Rhomberg]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62339/
---

Review request for geode.


Repository: geode


Description
---

Will this preserve backwards compat?


Diffs
-

  geode-core/src/main/java/org/apache/geode/security/ResourcePermission.java 
6a920d7c217f7383e38a7465540cdc0d542b6c81 


Diff: https://reviews.apache.org/r/62339/diff/1/


Testing
---


Thanks,

Patrick Rhomberg

Re: [Discuss] Moving away from virtual CacheableStringPtr toString() for Serializable objects in debug and logging to std::string and std::wstring

[David Kimura]

Seems like a good idea.

Here's a quick thought - I think c++11 is not really an obj.toString() kind
of language (like Java or C#).  Would it make sense to add std::to_string()
and std::to_wstring() equivalents for CacheableString?  This might mean
implementing following functions:

std::string to_string(CacheableString value);
std::wstring to_wstring(CacheableString value);

So that we can do something like:

std::cout << std::to_wstring(pdxser);

Thanks,
David

On Thu, Sep 14, 2017 at 11:10 AM, Michael William Dodge 
wrote:

> +1 for std::string and std::wstring.
>
> Sarge
>
> > On 14 Sep, 2017, at 11:10, Mark Hanson  wrote:
> >
> > Hi All,
> >
> > I wanted to broach the subject of moving away from moving away from
> CacheableStringPtrs for the toString representation of Serializable. It
> would seem desirable to move to std::string and std::wstring to use more
> basic types that would be faster to log and the code would be simpler for a
> user.
> >
> > Are there any opinions on this subject?
> >
> > Here is a before and after look at a chunk of code
> >
> > Before
> >
> > CacheableStringPtr ptr = pdxser->toString();
> > if (ptr->isWideString()) {
> >  printf(" query idx %d pulled object %S  :: \n", i,
> > ptr->asWChar());
> > } else {
> >  printf(" query idx %d pulled object %s  :: \n", i,
> > ptr->asChar());
> > }
> >
> > After
> >
> >
> > if (pdxser->isWideString()) {
> >   std::cout << " query idx “ << i << "pulled object ” <<
> pdxser->toWString() << std::endl;
> > } else {
> >   std::cout << " query idx “ << i << "pulled object ” <<
> pdxser->toString() << std::endl;
> > }
> >
> >
> > Thanks,
> > Mark
>
>

Re: [DISCUSS] authorizing function execution

[Swapnil Bawaskar]

Indeed, the function author has a higher level of privilege than someone
who is invoking a function, that is why the proposal here is to let the
function author choose what level of permissions are required to invoke a
function.

On Thu, Sep 14, 2017 at 11:20 AM Anilkumar Gingade 
wrote:

> >> from reaching into internal classes
> If thats the case; one could do anything, even with read permission...Isn't
> it...
>
>
>
> On Thu, Sep 14, 2017 at 10:43 AM, Jared Stewart 
> wrote:
>
> > There is nothing to prevent code in a function that's executing on a
> > server from reaching into internal classes and bypassing the public
> region
> > APIs. I think a function's author should ultimately determine the
> > permissions required to execute it.
> >
> > > On Sep 14, 2017, at 10:34 AM, Anilkumar Gingade 
> > wrote:
> > >
> > > When a function is accessing/modifying region; the function will be
> doing
> > > so by region apis, don't we have credential check with region apis; if
> > not
> > > can we add those checks here...instead of having it in the function...
> > >
> > > -Anil.
> > >
> > >
> > >
> > > On Wed, Sep 13, 2017 at 11:22 AM, Jared Stewart 
> > wrote:
> > >
> > >> After some more investigation into the implementation details, here is
> > our
> > >> updated proposal to add to the Function interface:
> > >>
> > >> default Collection getRequiredPermissions(
> > Optional
> > >> onRegion) {
> > >>  return Collections.singletonList(ResourcePermissions.DATA_WRITE);
> > >> }
> > >>
> > >> This method can be overridden by Function authors who want to require
> > >> permissions other than DATA:WRITE.. The onRegion parameter will be
> > present
> > >> only when a Function is executed via FunctionService.onRegion, and is
> > >> intended to allow Function authors to require different permissions
> > >> depending on the Region which Function will be executed on.  We pass
> the
> > >> region name into this method rather than the full FunctionContext
> > because
> > >> the latter would be much more expansive to implement.
> > >>
> > >> Any feedback is appreciated.
> > >>
> > >> Thanks,
> > >> Jared
> > >>
> > >>> On Aug 17, 2017, at 1:42 AM, Swapnil Bawaskar 
> > >> wrote:
> > >>>
> > >>> Discuss fix for GEODE-2817
> > >>> 
> > >>>
> > >>> Currently to execute a function, you will need "data:write"
> permission,
> > >> but
> > >>> it really depends on what the function is doing. For example, if a
> > >> function
> > >>> is just reading data, the function author might want users with
> > DATA:READ
> > >>> permissions to execute the function. The two options mentioned in the
> > >>> ticket are:
> > >>>
> > >>> 1) externalize SecurityService so that function author can use it in
> > the
> > >>> function.execute code to check authorization.
> > >>> 2) add a method to function interface to tell the framework what
> > >> permission
> > >>> this function needs to execute, so that the framework will check the
> > >>> permission before executing the function.
> > >>>
> > >>> I vote for #2 because, I think, a function author will be able to
> > easily
> > >>> discover a method on the Function interface, rather than trying to
> look
> > >> for
> > >>> SecurityService.
> > >>>
> > >>> I propose that we add the following new method to Function:
> > >>>
> > >>> default public List requiredPermissions() {
> > >>>  // default DATA:WRITE
> > >>> }
> > >>>
> > >>> In order to preserve existing behavior, the default required
> permission
> > >>> would be DATA:WRITE.
> > >>
> > >>
> >
> >
>

Re: [DISCUSS] authorizing function execution

[Anilkumar Gingade]

>> from reaching into internal classes
If thats the case; one could do anything, even with read permission...Isn't
it...



On Thu, Sep 14, 2017 at 10:43 AM, Jared Stewart  wrote:

> There is nothing to prevent code in a function that's executing on a
> server from reaching into internal classes and bypassing the public region
> APIs. I think a function's author should ultimately determine the
> permissions required to execute it.
>
> > On Sep 14, 2017, at 10:34 AM, Anilkumar Gingade 
> wrote:
> >
> > When a function is accessing/modifying region; the function will be doing
> > so by region apis, don't we have credential check with region apis; if
> not
> > can we add those checks here...instead of having it in the function...
> >
> > -Anil.
> >
> >
> >
> > On Wed, Sep 13, 2017 at 11:22 AM, Jared Stewart 
> wrote:
> >
> >> After some more investigation into the implementation details, here is
> our
> >> updated proposal to add to the Function interface:
> >>
> >> default Collection getRequiredPermissions(
> Optional
> >> onRegion) {
> >>  return Collections.singletonList(ResourcePermissions.DATA_WRITE);
> >> }
> >>
> >> This method can be overridden by Function authors who want to require
> >> permissions other than DATA:WRITE.. The onRegion parameter will be
> present
> >> only when a Function is executed via FunctionService.onRegion, and is
> >> intended to allow Function authors to require different permissions
> >> depending on the Region which Function will be executed on.  We pass the
> >> region name into this method rather than the full FunctionContext
> because
> >> the latter would be much more expansive to implement.
> >>
> >> Any feedback is appreciated.
> >>
> >> Thanks,
> >> Jared
> >>
> >>> On Aug 17, 2017, at 1:42 AM, Swapnil Bawaskar 
> >> wrote:
> >>>
> >>> Discuss fix for GEODE-2817
> >>> 
> >>>
> >>> Currently to execute a function, you will need "data:write" permission,
> >> but
> >>> it really depends on what the function is doing. For example, if a
> >> function
> >>> is just reading data, the function author might want users with
> DATA:READ
> >>> permissions to execute the function. The two options mentioned in the
> >>> ticket are:
> >>>
> >>> 1) externalize SecurityService so that function author can use it in
> the
> >>> function.execute code to check authorization.
> >>> 2) add a method to function interface to tell the framework what
> >> permission
> >>> this function needs to execute, so that the framework will check the
> >>> permission before executing the function.
> >>>
> >>> I vote for #2 because, I think, a function author will be able to
> easily
> >>> discover a method on the Function interface, rather than trying to look
> >> for
> >>> SecurityService.
> >>>
> >>> I propose that we add the following new method to Function:
> >>>
> >>> default public List requiredPermissions() {
> >>>  // default DATA:WRITE
> >>> }
> >>>
> >>> In order to preserve existing behavior, the default required permission
> >>> would be DATA:WRITE.
> >>
> >>
>
>

Re: [Discuss] Moving away from virtual CacheableStringPtr toString() for Serializable objects in debug and logging to std::string and std::wstring

[Michael William Dodge]

+1 for std::string and std::wstring.

Sarge

> On 14 Sep, 2017, at 11:10, Mark Hanson  wrote:
> 
> Hi All,
> 
> I wanted to broach the subject of moving away from moving away from 
> CacheableStringPtrs for the toString representation of Serializable. It would 
> seem desirable to move to std::string and std::wstring to use more basic 
> types that would be faster to log and the code would be simpler for a user.
> 
> Are there any opinions on this subject? 
> 
> Here is a before and after look at a chunk of code
> 
> Before
> 
> CacheableStringPtr ptr = pdxser->toString();
> if (ptr->isWideString()) {
>  printf(" query idx %d pulled object %S  :: \n", i,
> ptr->asWChar());
> } else {
>  printf(" query idx %d pulled object %s  :: \n", i,
> ptr->asChar());
> }
> 
> After
> 
> 
> if (pdxser->isWideString()) {
>   std::cout << " query idx “ << i << "pulled object ” <<  pdxser->toWString() 
> << std::endl;
> } else {
>   std::cout << " query idx “ << i << "pulled object ” <<  pdxser->toString() 
> << std::endl;
> }
> 
> 
> Thanks,
> Mark

[Discuss] Moving away from virtual CacheableStringPtr toString() for Serializable objects in debug and logging to std::string and std::wstring

[Mark Hanson]

Hi All,

I wanted to broach the subject of moving away from moving away from 
CacheableStringPtrs for the toString representation of Serializable. It would 
seem desirable to move to std::string and std::wstring to use more basic types 
that would be faster to log and the code would be simpler for a user.

Are there any opinions on this subject? 

Here is a before and after look at a chunk of code

Before

CacheableStringPtr ptr = pdxser->toString();
if (ptr->isWideString()) {
  printf(" query idx %d pulled object %S  :: \n", i,
 ptr->asWChar());
} else {
  printf(" query idx %d pulled object %s  :: \n", i,
 ptr->asChar());
}

After


if (pdxser->isWideString()) {
   std::cout << " query idx “ << i << "pulled object ” <<  pdxser->toWString() 
<< std::endl;
} else {
   std::cout << " query idx “ << i << "pulled object ” <<  pdxser->toString() 
<< std::endl;
}


Thanks,
Mark

Re: RestServersJUnitTest flickering tests

[Kirk Lund]

Please don't forget that we are NOT supposed to ignore failures in
FlakyTest. That was never the intention of FlakyTest. The intention was
this:

1) run the test under a testing target that ALWAYS forks (or forks every 30
distributed test cases) -- this was enough to stabilize many of these
flickering tests, presumably because they use short sleeps or waits and
sharing the JVM with other tests was causing GC pauses that caused the test
to fail

2) move ALL flickering tests to one target so that we don't waste time
analyzing flickering tests in all of the gradle targets

3) moving a flickering test to FlakyTest is meant to be TEMPORARY -- think
of it as better than @Ignore or Assume because it's still running and
should NOT be ignored but it gives you a bit of breathing room to fix it

Using @Ignore or non-OS-Assume is much worse than using FlakyTest because
the test continues to run under FlakyTest and should never be ignored by a
developer running precheckin.

Everyone, please don't ignore FlakyTest failures! If a flickering test
moves from IntegrationTest to FlakyTest and then continues to flicker, then
fixing that test needs to become a high priority. If the test stops
flickering (because it's running in a clean JVM) then fixing it is low
priority but still needs to be planned for.

We should have a strict policy that if a flickering test continues
flickering for a week or more then it should be moved to FlakyTest and
scheduled to be fixed. Also, if a flickering test moves to FlakyTest and
continues to fail a couple times a week then fixing it should become a high
priority -- it's not excuse to just ignore that test and it never was.

I honestly don't know how so many people ended up with the wrong
understanding of what FlakyTest is all about. It's all about keeping the
non-FlakyTest targets 100% GREEN(!), fixing the flakiness ASAP and moving
them back to their original target after they are fixed. Everyone should
know that if a FlakyTest fails for them in precheckin, then you cannot
ignore that -- you must investigate to find out what the cause is -- it may
be that the developer has an uncommitted change that introduces a bug which
causes a previously flickering test to start failing consistently. Anyone
who ignores this is not doing their job correctly.

Thanks,
Kirk

On Thu, Sep 14, 2017 at 10:30 AM, Patrick Rhomberg 
wrote:

> These problems stem from a dirty testing environment occupying the default
> port, which this test wants to use.  I have a pull request open to ignore
> the test when the default port is not available.
>
> On the one hand, an ignored test is essentially dead code.  If the test is
> consistently skipped and not only occasionally skipped, it's not informing
> us of anything.  But on the other hand, a flaky test ends up being more or
> less ignored and its errors end up being discarded as general flakiness.
>
> I'd favor the "assume X or skip" route over marking a test flaky, but that
> will require some diligence on our part to make sure these tests do end up
> eventually running.
>
> Pull request #771: https://github.com/apache/geode/pull/771
>
> On Thu, Sep 14, 2017 at 8:51 AM, Kirk Lund  wrote:
>
> > These tests have been failing intermittently in the integrationTest
> target
> > for over a week. I'm going to add the category FlakyTest to these tests.
> > See GEODE-3426.
> >
> > org.apache.geode.rest.internal.web.RestServersJUnitTest > testGet FAILED
> > org.junit.ComparisonFailure: expected:<[200]> but was:<[404]>
> > at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native
> > Method)
> > at
> > sun.reflect.NativeConstructorAccessorImpl.newInstance(
> > NativeConstructorAccessorImpl.java:62)
> > at
> > sun.reflect.DelegatingConstructorAccessorImpl.newInstance(
> > DelegatingConstructorAccessorImpl.java:45)
> > at
> > org.apache.geode.rest.internal.web.RestServersJUnitTest.testGet(
> > RestServersJUnitTest.java:49)
> >
> > org.apache.geode.rest.internal.web.RestServersJUnitTest >
> > testServerStartedOnDefaultPort FAILED
> > org.json.JSONException: Value  of type java.lang.String cannot
> be
> > converted to JSONArray
> > at org.json.JSON.typeMismatch(JSON.java:108)
> > at org.json.JSONArray.(JSONArray.java:85)
> > at
> > org.apache.geode.rest.internal.web.GeodeRestClient.
> > getJsonArray(GeodeRestClient.java:99)
> > at
> > org.apache.geode.rest.internal.web.RestServersJUnitTest.
> > testServerStartedOnDefaultPort(RestServersJUnitTest.java:55)
> >
>

Re: [DISCUSS] authorizing function execution

[Jared Stewart]

There is nothing to prevent code in a function that's executing on a server 
from reaching into internal classes and bypassing the public region APIs. I 
think a function's author should ultimately determine the permissions required 
to execute it.   

> On Sep 14, 2017, at 10:34 AM, Anilkumar Gingade  wrote:
> 
> When a function is accessing/modifying region; the function will be doing
> so by region apis, don't we have credential check with region apis; if not
> can we add those checks here...instead of having it in the function...
> 
> -Anil.
> 
> 
> 
> On Wed, Sep 13, 2017 at 11:22 AM, Jared Stewart  wrote:
> 
>> After some more investigation into the implementation details, here is our
>> updated proposal to add to the Function interface:
>> 
>> default Collection 
>> getRequiredPermissions(Optional
>> onRegion) {
>>  return Collections.singletonList(ResourcePermissions.DATA_WRITE);
>> }
>> 
>> This method can be overridden by Function authors who want to require
>> permissions other than DATA:WRITE.. The onRegion parameter will be present
>> only when a Function is executed via FunctionService.onRegion, and is
>> intended to allow Function authors to require different permissions
>> depending on the Region which Function will be executed on.  We pass the
>> region name into this method rather than the full FunctionContext because
>> the latter would be much more expansive to implement.
>> 
>> Any feedback is appreciated.
>> 
>> Thanks,
>> Jared
>> 
>>> On Aug 17, 2017, at 1:42 AM, Swapnil Bawaskar 
>> wrote:
>>> 
>>> Discuss fix for GEODE-2817
>>> 
>>> 
>>> Currently to execute a function, you will need "data:write" permission,
>> but
>>> it really depends on what the function is doing. For example, if a
>> function
>>> is just reading data, the function author might want users with DATA:READ
>>> permissions to execute the function. The two options mentioned in the
>>> ticket are:
>>> 
>>> 1) externalize SecurityService so that function author can use it in the
>>> function.execute code to check authorization.
>>> 2) add a method to function interface to tell the framework what
>> permission
>>> this function needs to execute, so that the framework will check the
>>> permission before executing the function.
>>> 
>>> I vote for #2 because, I think, a function author will be able to easily
>>> discover a method on the Function interface, rather than trying to look
>> for
>>> SecurityService.
>>> 
>>> I propose that we add the following new method to Function:
>>> 
>>> default public List requiredPermissions() {
>>>  // default DATA:WRITE
>>> }
>>> 
>>> In order to preserve existing behavior, the default required permission
>>> would be DATA:WRITE.
>> 
>> 

Re: [DISCUSS] authorizing function execution

[Anilkumar Gingade]

When a function is accessing/modifying region; the function will be doing
so by region apis, don't we have credential check with region apis; if not
can we add those checks here...instead of having it in the function...

-Anil.



On Wed, Sep 13, 2017 at 11:22 AM, Jared Stewart  wrote:

> After some more investigation into the implementation details, here is our
> updated proposal to add to the Function interface:
>
> default Collection getRequiredPermissions(Optional
> onRegion) {
>   return Collections.singletonList(ResourcePermissions.DATA_WRITE);
> }
>
> This method can be overridden by Function authors who want to require
> permissions other than DATA:WRITE.. The onRegion parameter will be present
> only when a Function is executed via FunctionService.onRegion, and is
> intended to allow Function authors to require different permissions
> depending on the Region which Function will be executed on.  We pass the
> region name into this method rather than the full FunctionContext because
> the latter would be much more expansive to implement.
>
> Any feedback is appreciated.
>
> Thanks,
> Jared
>
> > On Aug 17, 2017, at 1:42 AM, Swapnil Bawaskar 
> wrote:
> >
> > Discuss fix for GEODE-2817
> > 
> >
> > Currently to execute a function, you will need "data:write" permission,
> but
> > it really depends on what the function is doing. For example, if a
> function
> > is just reading data, the function author might want users with DATA:READ
> > permissions to execute the function. The two options mentioned in the
> > ticket are:
> >
> > 1) externalize SecurityService so that function author can use it in the
> > function.execute code to check authorization.
> > 2) add a method to function interface to tell the framework what
> permission
> > this function needs to execute, so that the framework will check the
> > permission before executing the function.
> >
> > I vote for #2 because, I think, a function author will be able to easily
> > discover a method on the Function interface, rather than trying to look
> for
> > SecurityService.
> >
> > I propose that we add the following new method to Function:
> >
> > default public List requiredPermissions() {
> >   // default DATA:WRITE
> > }
> >
> > In order to preserve existing behavior, the default required permission
> > would be DATA:WRITE.
>
>

[CANCEL][VOTE] Apache Geode release - v1.2.1 RC3

[Anthony Baker]

Cancelling due to problems in the source distributions (the official release 
bits).

Anthony

> On Sep 11, 2017, at 2:03 PM, Anthony Baker  wrote:
> 
> This is the third release candidate for Apache Geode, version 1.2.1.
> Thanks to all the community members for their contributions to this
> release!
> 
> *** Please download, test and vote by Thursday, September 14, 1200 hrs
> US Pacific. ***
> 
> It fixes the following issues:
>  
> https://issues.apache.org/jira/secure/ReleaseNote.jspa?projectId=12318420=12341124
> 
> Note that we are voting upon the source tags:  rel/v1.2.1.RC3
>  https://github.com/apache/geode/tree/rel/v1.2.1.RC3
>  https://github.com/apache/geode-examples/tree/rel/v1.2.1.RC3
> 
> Commit ID:
>  0b881b515eb1dcea974f0f5c1b40da03d42af9cf (geode)
>  5d034de588a43cdefb8fbb3a6259579785768340 (geode-examples)
> 
> Source and binary files:
>  https://dist.apache.org/repos/dist/dev/geode/1.2.1.RC3
> 
> Maven staging repo:
>  https://repository.apache.org/content/repositories/orgapachegeode-1023
> 
> Geode's KEYS file containing PGP keys we use to sign the release:
>  https://github.com/apache/geode/blob/develop/KEYS
> 
> pub  4096R/C72CFB64 2015-10-01
>  Fingerprint=948E 8234 14BE 693A 7F74  ABBE 19DB CAEE C72C FB64
> 
> 
> Anthony

Re: [DISCUSS] GEODE-3617 Replace gemfire prefix with geode

[Kirk Lund]

That's a bigger change and I'm not sure how you would handle backwards
compatibility for users using gemfire.properties.

On Thu, Sep 14, 2017 at 9:15 AM, Jacob Barrett  wrote:

> Or better yet, we stop using properties files already.
>
> On Thu, Sep 14, 2017 at 8:55 AM Dave Barnes  wrote:
>
> > Is there a possibility that the code might find its way into additional
> > contexts with other names? If so, perhaps we should consider a more
> generic
> > identifier, such as PRODUCT_PREFIX.
> >
> > On Thu, Sep 14, 2017 at 4:42 AM, Dinesh Akhand 
> > wrote:
> >
> > > Hi,
> > >
> > > Why we are keeping gemfire in current geode 1.2 , Can we replace this
> > with
> > > GEODE
> > > File : DistributionConfig.java
> > >
> > > Current code:
> > >   String GEMFIRE_PREFIX = "gemfire.";
> > >
> > > Suggestion to change:
> > >  String GEODE_PREFIX = "geode.";
> > >
> > > Why do you think ?
> > > Can we go ahead  and change this ?
> > > It will impact lots of files & all configuration will be now using with
> > > geode.
> > >
> > > Thanks,
> > > Dinesh Akhand
> > > This message and the information contained herein is proprietary and
> > > confidential and subject to the Amdocs policy statement,
> > >
> > > you may review at https://www.amdocs.com/about/email-disclaimer <
> > > https://www.amdocs.com/about/email-disclaimer>
> > >
> >
>

Re: [DISCUSS] GEODE-3617 Replace gemfire prefix with geode

[Jacob Barrett]

Or better yet, we stop using properties files already.

On Thu, Sep 14, 2017 at 8:55 AM Dave Barnes  wrote:

> Is there a possibility that the code might find its way into additional
> contexts with other names? If so, perhaps we should consider a more generic
> identifier, such as PRODUCT_PREFIX.
>
> On Thu, Sep 14, 2017 at 4:42 AM, Dinesh Akhand 
> wrote:
>
> > Hi,
> >
> > Why we are keeping gemfire in current geode 1.2 , Can we replace this
> with
> > GEODE
> > File : DistributionConfig.java
> >
> > Current code:
> >   String GEMFIRE_PREFIX = "gemfire.";
> >
> > Suggestion to change:
> >  String GEODE_PREFIX = "geode.";
> >
> > Why do you think ?
> > Can we go ahead  and change this ?
> > It will impact lots of files & all configuration will be now using with
> > geode.
> >
> > Thanks,
> > Dinesh Akhand
> > This message and the information contained herein is proprietary and
> > confidential and subject to the Amdocs policy statement,
> >
> > you may review at https://www.amdocs.com/about/email-disclaimer <
> > https://www.amdocs.com/about/email-disclaimer>
> >
>

Re: [DISCUSS] GEODE-3617 Replace gemfire prefix with geode

[Kirk Lund]

Whoever takes this work on will need to spend time figuring out a way to
make it pluggable and support multiple prefixes possibly with pluggable
order of preference. For example, we would need to support looking for
geode.properties and gemfire.properties or it would break backwards
compatibility for users.

On Thu, Sep 14, 2017 at 8:55 AM, Dave Barnes  wrote:

> Is there a possibility that the code might find its way into additional
> contexts with other names? If so, perhaps we should consider a more generic
> identifier, such as PRODUCT_PREFIX.
>
> On Thu, Sep 14, 2017 at 4:42 AM, Dinesh Akhand 
> wrote:
>
> > Hi,
> >
> > Why we are keeping gemfire in current geode 1.2 , Can we replace this
> with
> > GEODE
> > File : DistributionConfig.java
> >
> > Current code:
> >   String GEMFIRE_PREFIX = "gemfire.";
> >
> > Suggestion to change:
> >  String GEODE_PREFIX = "geode.";
> >
> > Why do you think ?
> > Can we go ahead  and change this ?
> > It will impact lots of files & all configuration will be now using with
> > geode.
> >
> > Thanks,
> > Dinesh Akhand
> > This message and the information contained herein is proprietary and
> > confidential and subject to the Amdocs policy statement,
> >
> > you may review at https://www.amdocs.com/about/email-disclaimer <
> > https://www.amdocs.com/about/email-disclaimer>
> >
>

Re: [DISCUSS] geode-native c++ exceptions

[Jacob Barrett]

The problem stems from the fact that none of the std exceptions virtually
inherit from std::exception so you end up in the inheritance triangle of
love. I say we avoid the multiple inheritance issues with exceptions by
avoiding multiple inheritance altogether in exceptions. See this example.
http://coliru.stacked-crooked.com/a/2e32eb021e85

Basically all of our exceptions extend our Exception class which extends
std::exception. None extend any of the other std exceptions, like
bad_alloc, etc. The downside is you can't catch any std exception other
than std::exception but the upside is that this actually works. There were
also very few exceptions that actually could extend any std exceptions
beyond std::exception.

-Jake


On Wed, Sep 13, 2017 at 10:52 PM Jacob Barrett  wrote:

> Let me see if I can considerate the consensus here:
>
> As reasonably possible throw library specific exceptions from all public
> methods.
>
> As reasonably possible those exceptions should extend C++11 standard
> library exceptions.
>
> All exceptions should extend less specific but logically relevant base
> exceptions.
>
> A few quick examples:
> namespace apache {
> namespace geode {
> namespace client {
>
> class Exception : public std::exception {...};
>
> class IllegalArgumentException : public Exception, public
> std::invalid_argument {...};
>
> class TransactionException : public Exception {...};
>
> class RollbackException : public TransactionException {...};
>
> }
> }
> }
>
> Additionally, investigate using Boost Stack Track library for providing
> stack context in exceptions, otherwise dump the current stack tracing
> feature that is incomplete and very platform specific.
>
> Does anyone have a different understanding of the consensus?
>
>
> I found some problems with this model. The IllegalArgumentException would
> not be caught in a catch (const std::exception& e) statement do to
> multiple inheritance. Another nasty is std::exception doesn't have a
> constructor to populate the what() value. Some examples of this problem
> can be seen here:
> http://coliru.stacked-crooked.com/a/5b6e34c7ea020fc8
>
>
> -Jake
>
>
>
>
> On Wed, Sep 13, 2017 at 4:37 PM Mark Hanson  wrote:
>
>> I think that it would be best to abide by using the std::exception as the
>> base. I think it brings with it all the language support and flexibility.
>> There are a few penalties obviously to using std::exception as a base, but
>> I think they are sufficiently offset by using a standard. As far as the
>> number of exceptions, I believe in the philosophy of using as few as
>> possible and using inheritance to drive specificity. The benefit is that
>> the code can be as simple or as complex as it can be without unnecessary
>> overhead e.g error => network error => tcp error. So, I may just care there
>> is a network error and the being able to tell if something derives from
>> network error is perfect.
>>
>> Thanks,
>> Mark
>>
>>
>> > On Sep 8, 2017, at 1:35 PM, Ernest Burghardt 
>> wrote:
>> >
>> > if we continue the merging of Jake <> Sarge comments we might find that
>> > std::exception(s) is sufficient if the many name exceptions that
>> pertain to
>> > the Library are all handled in a similar manner and only have unique
>> names
>> > for human readability, but not a functional consideration...
>> >
>> > EB
>> >
>> > On Fri, Sep 8, 2017 at 8:52 AM, Michael William Dodge <
>> mdo...@pivotal.io>
>> > wrote:
>> >
>> >> I subscribe to Josh Gray's philosophy of only having another exception
>> >> class if there is something different to be done when it's caught. For
>> >> example, if the caller would do the exact same thing for
>> >> NoPermissionsException and DiskFullException, just use an IOException
>> and
>> >> be done with it. I also subscribe to the philosophy that a library
>> have its
>> >> own exception hierarchy (possibly with a single class), which I think
>> >> meshes with Jake's "exceptions exiting a library to be reasonably
>> limited
>> >> to exceptions generated by and relating to that library".
>> >>
>> >> Sarge
>> >>
>> >>> On 8 Sep, 2017, at 07:19, Jacob Barrett  wrote:
>> >>>
>> >>> Sorry for jumping on this thread so late. This is an important issue
>> we
>> >>> need to address.
>> >>>
>> >>> On Thu, Aug 17, 2017 at 11:57 AM David Kimura 
>> >> wrote:
>> >>>
>>  Using exceptions seems contrary to the Google C++ Style Guide we
>> >> adopted,
>>  which states: *"do not use C++ exceptions"* [3
>>  ].
>> Here
>> >> is
>>  a
>>  link [4 ] to a
>>  cppcon
>>  talk defending their decision.  Does it make sense to enforce this
>> rule
>> >> on
>>  our code base?
>> 
>> >>>
>> >>> I don't agree with this approach, as I always tend towards
>> >>> progress/modernization, but it was 

RestServersJUnitTest flickering tests

[Kirk Lund]

These tests have been failing intermittently in the integrationTest target
for over a week. I'm going to add the category FlakyTest to these tests.
See GEODE-3426.

org.apache.geode.rest.internal.web.RestServersJUnitTest > testGet FAILED
org.junit.ComparisonFailure: expected:<[200]> but was:<[404]>
at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native
Method)
at
sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
at
sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
at
org.apache.geode.rest.internal.web.RestServersJUnitTest.testGet(RestServersJUnitTest.java:49)

org.apache.geode.rest.internal.web.RestServersJUnitTest >
testServerStartedOnDefaultPort FAILED
org.json.JSONException: Value  of type java.lang.String cannot be
converted to JSONArray
at org.json.JSON.typeMismatch(JSON.java:108)
at org.json.JSONArray.(JSONArray.java:85)
at
org.apache.geode.rest.internal.web.GeodeRestClient.getJsonArray(GeodeRestClient.java:99)
at
org.apache.geode.rest.internal.web.RestServersJUnitTest.testServerStartedOnDefaultPort(RestServersJUnitTest.java:55)

Build failed in Jenkins: Geode-nightly #954

[Apache Jenkins Server]

See 


Changes:

[github] GEODE-3580: patch test to avoid the current failure. (#774)

[github] GEODE-3579 Update gfsh stop locator docs (#765)

[jstewart] GEODE-3544: Fix JSON parsing error

[jstewart] Update javadoc for JarBuilder

[khowe] GEODE-3539: Add test for missing coverage of status locator command

[bschuchardt] GEODE-3562 CI Failure: JSONCodecJUnitTest.testSimpleJSONEncode 
fails on

[kohlmu-pivotal] GEODE-3079 Ensure emergencyClose() closes socket

--
[...truncated 114.93 KB...]
Note: Recompile with -Xlint:unchecked for details.

:geode-cq:processTestResources
:geode-cq:testClasses
:geode-cq:checkMissedTests
:geode-cq:spotlessJavaCheck
:geode-cq:spotlessCheck
:geode-cq:test
:geode-cq:check
:geode-cq:build
:geode-cq:distributedTest
:geode-cq:integrationTest
:geode-json:assemble
:geode-json:compileTestJava NO-SOURCE
:geode-json:processTestResources
:geode-json:testClasses
:geode-json:checkMissedTests NO-SOURCE
:geode-json:spotlessJavaCheck
:geode-json:spotlessCheck
:geode-json:test NO-SOURCE
:geode-json:check
:geode-json:build
:geode-json:distributedTest NO-SOURCE
:geode-json:integrationTest NO-SOURCE
:geode-junit:javadoc
:geode-junit:javadocJar
:geode-junit:sourcesJar
:geode-junit:signArchives SKIPPED
:geode-junit:assemble
:geode-junit:compileTestJavaNote: 

 uses or overrides a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
Note: 

 uses unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.

:geode-junit:processTestResources
:geode-junit:testClasses
:geode-junit:checkMissedTests
:geode-junit:spotlessJavaCheck
:geode-junit:spotlessCheck
:geode-junit:test
:geode-junit:check
:geode-junit:build
:geode-junit:distributedTest
:geode-junit:integrationTest
:geode-lucene:assemble
:geode-lucene:compileTestJavaNote: Some input files use or override a 
deprecated API.
Note: Recompile with -Xlint:deprecation for details.
Note: Some input files use unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.

:geode-lucene:processTestResources
:geode-lucene:testClasses
:geode-lucene:checkMissedTests
:geode-lucene:spotlessJavaCheck
:geode-lucene:spotlessCheck
:geode-lucene:test
:geode-lucene:check
:geode-lucene:build
:geode-lucene:distributedTest
:geode-lucene:integrationTest
:geode-old-client-support:assemble
:geode-old-client-support:compileTestJavaNote: 

 uses or overrides a deprecated API.
Note: Recompile with -Xlint:deprecation for details.

:geode-old-client-support:processTestResources NO-SOURCE
:geode-old-client-support:testClasses
:geode-old-client-support:checkMissedTests
:geode-old-client-support:spotlessJavaCheck
:geode-old-client-support:spotlessCheck
:geode-old-client-support:test
:geode-old-client-support:check
:geode-old-client-support:build
:geode-old-client-support:distributedTest
:geode-old-client-support:integrationTest
:geode-old-versions:distributedTest NO-SOURCE
:geode-old-versions:integrationTest NO-SOURCE
:geode-protobuf:assemble
:geode-protobuf:extractIncludeTestProto
:geode-protobuf:extractTestProto UP-TO-DATE
:geode-protobuf:generateTestProto NO-SOURCE
:geode-protobuf:compileTestJavaNote: 

 uses or overrides a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
Note: Some input files use unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.

:geode-protobuf:processTestResources
:geode-protobuf:testClasses
:geode-protobuf:checkMissedTests
:geode-protobuf:spotlessJavaCheck
:geode-protobuf:spotlessCheck
:geode-protobuf:test
:geode-protobuf:check
:geode-protobuf:build
:geode-protobuf:distributedTest
:geode-protobuf:integrationTest
:geode-pulse:assemble
:geode-pulse:compileTestJavaNote: Some input files use or override a deprecated 
API.
Note: Recompile with -Xlint:deprecation for details.
Note: 

 uses unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.

:geode-pulse:processTestResources
:geode-pulse:testClasses
:geode-pulse:checkMissedTests
:geode-pulse:spotlessJavaCheck
:geode-pulse:spotlessCheck
:geode-pulse:test
:geode-pulse:check
:geode-pulse:build
:geode-pulse:distributedTest
:geode-pulse:integrationTest
:geode-rebalancer:assemble

[DISCUSS] GEODE-3617 Replace gemfire prefix with geode

[Dinesh Akhand]

Hi,

Why we are keeping gemfire in current geode 1.2 , Can we replace this with GEODE
File : DistributionConfig.java

Current code:
  String GEMFIRE_PREFIX = "gemfire.";

Suggestion to change:
 String GEODE_PREFIX = "geode.";

Why do you think ?
Can we go ahead  and change this ?
It will impact lots of files & all configuration will be now using with geode.

Thanks,
Dinesh Akhand
This message and the information contained herein is proprietary and 
confidential and subject to the Amdocs policy statement,

you may review at https://www.amdocs.com/about/email-disclaimer 

Build failed in Jenkins: Geode-release #89

[Apache Jenkins Server]

See 

--
[...truncated 1.25 MB...]
:geode-junit:assemble
:geode-junit:compileTestJavaNote: 

 uses or overrides a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
Note: 

 uses unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.

:geode-junit:processTestResources
:geode-junit:testClasses
:geode-junit:checkMissedTests
:geode-junit:spotlessJavaCheck
:geode-junit:spotlessCheck
:geode-junit:test
:geode-junit:check
:geode-junit:build
:geode-junit:distributedTest
:geode-junit:integrationTest
:geode-lucene:assemble
:geode-lucene:compileTestJava
Download 
https://repo1.maven.org/maven2/org/apache/lucene/lucene-test-framework/6.4.1/lucene-test-framework-6.4.1.pom
Download 
https://repo1.maven.org/maven2/org/apache/lucene/lucene-codecs/6.4.1/lucene-codecs-6.4.1.pom
Download 
https://repo1.maven.org/maven2/org/apache/lucene/lucene-analyzers-phonetic/6.4.1/lucene-analyzers-phonetic-6.4.1.pom
Download 
https://repo1.maven.org/maven2/com/carrotsearch/randomizedtesting/randomizedtesting-runner/2.4.0/randomizedtesting-runner-2.4.0.pom
Download 
https://repo1.maven.org/maven2/com/carrotsearch/randomizedtesting/randomizedtesting-parent/2.4.0/randomizedtesting-parent-2.4.0.pom
Download 
https://repo1.maven.org/maven2/org/apache/lucene/lucene-test-framework/6.4.1/lucene-test-framework-6.4.1.jar
Download 
https://repo1.maven.org/maven2/org/apache/lucene/lucene-codecs/6.4.1/lucene-codecs-6.4.1.jar
Download 
https://repo1.maven.org/maven2/org/apache/lucene/lucene-analyzers-phonetic/6.4.1/lucene-analyzers-phonetic-6.4.1.jar
Download 
https://repo1.maven.org/maven2/com/carrotsearch/randomizedtesting/randomizedtesting-runner/2.4.0/randomizedtesting-runner-2.4.0.jar
Note: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
Note: Some input files use unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.
:geode-lucene:processTestResources
:geode-lucene:testClasses
:geode-lucene:checkMissedTests
:geode-lucene:spotlessJavaCheck
:geode-lucene:spotlessCheck
:geode-lucene:test
:geode-lucene:check
:geode-lucene:build
:geode-lucene:distributedTest
:geode-lucene:integrationTest
:geode-old-client-support:assemble
:geode-old-client-support:compileTestJavaNote: 

 uses or overrides a deprecated API.
Note: Recompile with -Xlint:deprecation for details.

:geode-old-client-support:processTestResources UP-TO-DATE
:geode-old-client-support:testClasses
:geode-old-client-support:checkMissedTests
:geode-old-client-support:spotlessJavaCheck
:geode-old-client-support:spotlessCheck
:geode-old-client-support:test
:geode-old-client-support:check
:geode-old-client-support:build
:geode-old-client-support:distributedTest
:geode-old-client-support:integrationTest
:geode-old-versions:javadoc UP-TO-DATE
:geode-old-versions:javadocJar
:geode-old-versions:sourcesJar
:geode-old-versions:signArchives SKIPPED
:geode-old-versions:assemble
:geode-old-versions:compileTestJava UP-TO-DATE
:geode-old-versions:processTestResources UP-TO-DATE
:geode-old-versions:testClasses UP-TO-DATE
:geode-old-versions:checkMissedTests UP-TO-DATE
:geode-old-versions:spotlessJavaCheck
:geode-old-versions:spotlessCheck
:geode-old-versions:test UP-TO-DATE
:geode-old-versions:check
:geode-old-versions:build
:geode-old-versions:distributedTest UP-TO-DATE
:geode-old-versions:integrationTest UP-TO-DATE
:geode-pulse:assemble
:geode-pulse:compileTestJava
Download 
https://repo1.maven.org/maven2/com/codeborne/phantomjsdriver/1.3.0/phantomjsdriver-1.3.0.pom
Download 
https://repo1.maven.org/maven2/org/springframework/spring-test/4.3.6.RELEASE/spring-test-4.3.6.RELEASE.pom
Download 
https://repo1.maven.org/maven2/com/codeborne/phantomjsdriver/1.3.0/phantomjsdriver-1.3.0.jar
Download 
https://repo1.maven.org/maven2/org/seleniumhq/selenium/selenium-api/3.0.1/selenium-api-3.0.1.jar
Download 
https://repo1.maven.org/maven2/org/seleniumhq/selenium/selenium-remote-driver/3.0.1/selenium-remote-driver-3.0.1.jar
Download 
https://repo1.maven.org/maven2/org/seleniumhq/selenium/selenium-support/3.0.1/selenium-support-3.0.1.jar
Download 
https://repo1.maven.org/maven2/org/springframework/spring-test/4.3.6.RELEASE/spring-test-4.3.6.RELEASE.jar
Note: 

 uses or overrides a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
Note: