You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@geode.apache.org by Hitesh Khamesra <hk...@pivotal.io> on 2016/11/18 00:41:38 UTC

Review Request 53867: GEODE-2129 make pdxtype id random(not sequential)

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

Review request for geode, Bruce Schuchardt and Udo Kohlmeyer.

Repository: geode

Description
-------

Right now pdxtype id has 4 bytes. Out of those 4 bytes, one byte reserved for distributed-system-id, this make sure type id generated from different cluster has different id. For rest of the three bytes we just increment counter to create new pdxtype id. In the field, we have observed that sometimes this pdxType Id collides. One reason could be they end up having same distributed-system-id for the different cluster.
Thus to avoid a collision, we will be using hashcode of pdxType for three bytes of pdxType id. That will reduce the possibility of collision.

Diffs
-----

geode-core/src/main/java/org/apache/geode/pdx/internal/EnumId.java 5d399eb
geode-core/src/main/java/org/apache/geode/pdx/internal/PdxType.java b586f64
geode-core/src/main/java/org/apache/geode/pdx/internal/PeerTypeRegistration.java 0226cca
geode-core/src/test/java/org/apache/geode/pdx/PdxAttributesJUnitTest.java c45abce
geode-core/src/test/java/org/apache/geode/pdx/PdxSerializableJUnitTest.java 5cd822c
geode-wan/src/test/java/org/apache/geode/internal/cache/wan/WANTestBase.java f9c18ec
geode-wan/src/test/java/org/apache/geode/internal/cache/wan/misc/PDXNewWanDUnitTest.java f1e8f42

Diff: https://reviews.apache.org/r/53867/diff/

Testing
-------

Thanks,

Hitesh Khamesra

Re: Review Request 53867: GEODE-2129 make pdxtype id random(not sequential)

Posted by Bruce Schuchardt <bs...@pivotal.io>.

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


Ship it!




Ship It!

- Bruce Schuchardt


On Nov. 22, 2016, 12:49 a.m., Hitesh Khamesra wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53867/
> -----------------------------------------------------------
> 
> (Updated Nov. 22, 2016, 12:49 a.m.)
> 
> 
> Review request for geode, Bruce Schuchardt and Udo Kohlmeyer.
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> Right now pdxtype id has 4 bytes. Out of those 4 bytes, one byte reserved for distributed-system-id, this make sure type id generated from different cluster has different id. For rest of the three bytes we just increment counter to create new pdxtype id. In the field, we have observed that sometimes this pdxType Id collides. One reason could be they end up having same distributed-system-id for the different cluster.
> Thus to avoid a collision, we will be using hashcode of pdxType for three bytes of pdxType id. That will reduce the possibility of collision.
> 
> 
> Diffs
> -----
> 
>   geode-core/src/main/java/org/apache/geode/pdx/internal/EnumId.java 5d399eb 
>   geode-core/src/main/java/org/apache/geode/pdx/internal/PdxType.java b586f64 
>   geode-core/src/main/java/org/apache/geode/pdx/internal/PeerTypeRegistration.java 0226cca 
>   geode-core/src/test/java/org/apache/geode/internal/cache/MultipleOplogsRollingFeatureJUnitTest.java c79f4fa 
>   geode-core/src/test/java/org/apache/geode/pdx/PdxAttributesJUnitTest.java c45abce 
>   geode-core/src/test/java/org/apache/geode/pdx/PdxSerializableJUnitTest.java 5cd822c 
>   geode-wan/src/test/java/org/apache/geode/internal/cache/wan/WANTestBase.java f9c18ec 
>   geode-wan/src/test/java/org/apache/geode/internal/cache/wan/misc/PDXNewWanDUnitTest.java f1e8f42 
> 
> Diff: https://reviews.apache.org/r/53867/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Hitesh Khamesra
> 
>

Re: Review Request 53867: GEODE-2129 make pdxtype id random(not sequential)

Posted by Hitesh Khamesra <hk...@pivotal.io>.

(Updated Nov. 22, 2016, 12:49 a.m.)

Review request for geode, Bruce Schuchardt and Udo Kohlmeyer.

Changes
-------

Made it propertly

Repository: geode

Description
-------

Diffs (updated)
-----

geode-core/src/main/java/org/apache/geode/pdx/internal/EnumId.java 5d399eb
geode-core/src/main/java/org/apache/geode/pdx/internal/PdxType.java b586f64
geode-core/src/main/java/org/apache/geode/pdx/internal/PeerTypeRegistration.java 0226cca
geode-core/src/test/java/org/apache/geode/internal/cache/MultipleOplogsRollingFeatureJUnitTest.java c79f4fa
geode-core/src/test/java/org/apache/geode/pdx/PdxAttributesJUnitTest.java c45abce
geode-core/src/test/java/org/apache/geode/pdx/PdxSerializableJUnitTest.java 5cd822c
geode-wan/src/test/java/org/apache/geode/internal/cache/wan/WANTestBase.java f9c18ec
geode-wan/src/test/java/org/apache/geode/internal/cache/wan/misc/PDXNewWanDUnitTest.java f1e8f42

Diff: https://reviews.apache.org/r/53867/diff/

Testing
-------

Thanks,

Hitesh Khamesra

Re: Review Request 53867: GEODE-2129 make pdxtype id random(not sequential)

Posted by Hitesh Khamesra <hk...@pivotal.io>.

(Updated Nov. 21, 2016, 11:56 p.m.)

Review request for geode, Bruce Schuchardt and Udo Kohlmeyer.

Changes
-------

New patch includes Bruces your suggestions. ANd another fix for CacheWriter NullPointerException

Repository: geode

Description
-------

Diffs (updated)
-----

Diff: https://reviews.apache.org/r/53867/diff/

Testing
-------

Thanks,

Hitesh Khamesra

Re: Review Request 53867: GEODE-2129 make pdxtype id random(not sequential)

Posted by Hitesh Khamesra <hi...@yahoo.com.INVALID>.

Agree Mike. Right this would help to recover the pdx data. 


      From: Michael Stolz <ms...@pivotal.io>
 To: dev@geode.apache.org; Hitesh Khamesra <hi...@yahoo.com> 
Cc: Hitesh Khamesra <hk...@pivotal.io>; Udo Kohlmeyer <uk...@gmail.com>; Bruce Schuchardt <bs...@pivotal.io>; geode <de...@geode.incubator.apache.org>
 Sent: Monday, November 21, 2016 6:07 AM
 Subject: Re: Review Request 53867: GEODE-2129 make pdxtype id random(not sequential)
   
I think we need to be more forceful about requiring different distributed
system id's. This idea of using hashcode seems like a debugging problem
waiting to happen.

--
Mike Stolz
Principal Engineer, GemFire Product Manager
Mobile: 631-835-4771

On Fri, Nov 18, 2016 at 6:39 PM, Hitesh Khamesra <
hiteshk25@yahoo.com.invalid> wrote:

> Before using hashcode for pdxType creation, we will check whether it is
> available or not. If it's not available then, we will use next available
> id. We have introduced this mechanism to harden the cases where the user
> ends up having two clusters with the same dsid.        From: Vishal Rao <
> vishalrao@gmail.com>
>  To: dev@geode.apache.org; Hitesh Khamesra <hk...@pivotal.io>
> Cc: Udo Kohlmeyer <uk...@gmail.com>; Bruce Schuchardt <
> bschuchardt@pivotal.io>; geode <de...@geode.incubator.apache.org>
>  Sent: Thursday, November 17, 2016 11:51 PM
>  Subject: Re: Review Request 53867: GEODE-2129 make pdxtype id random(not
> sequential)
>
> Apologies if this is noise, but wouldn't using the hash code actually
> increase the likelihood of ID collisions?
>
> On Fri, Nov 18, 2016 at 6:11 AM, Hitesh Khamesra <hk...@pivotal.io>
> wrote:
>
> >
> > -----------------------------------------------------------
> > This is an automatically generated e-mail. To reply, visit:
> > https://reviews.apache.org/r/53867/
> > -----------------------------------------------------------
> >
> > Review request for geode, Bruce Schuchardt and Udo Kohlmeyer.
> >
> >
> > Repository: geode
> >
> >
> > Description
> > -------
> >
> > Right now pdxtype id has 4 bytes. Out of those 4 bytes, one byte reserved
> > for distributed-system-id, this make sure type id generated from
> different
> > cluster has different id. For rest of the three bytes we just increment
> > counter to create new pdxtype id. In the field, we have observed that
> > sometimes this pdxType Id collides. One reason could be they end up
> having
> > same distributed-system-id for the different cluster.
> > Thus to avoid a collision, we will be using hashcode of pdxType for three
> > bytes of pdxType id. That will reduce the possibility of collision.
> >
> >
> > Diffs
> > -----
> >
> >  geode-core/src/main/java/org/apache/geode/pdx/internal/EnumId.java
> > 5d399eb
> >  geode-core/src/main/java/org/apache/geode/pdx/internal/PdxType.java
> > b586f64
> >  geode-core/src/main/java/org/apache/geode/pdx/internal/
> PeerTypeRegistration.java
> > 0226cca
> >  geode-core/src/test/java/org/apache/geode/pdx/
> PdxAttributesJUnitTest.java
> > c45abce
> >  geode-core/src/test/java/org/apache/geode/pdx/
> PdxSerializableJUnitTest.java
> > 5cd822c
> >  geode-wan/src/test/java/org/apache/geode/internal/cache/
> wan/WANTestBase.java
> > f9c18ec
> >  geode-wan/src/test/java/org/apache/geode/internal/cache/
> > wan/misc/PDXNewWanDUnitTest.java f1e8f42
> >
> > Diff: https://reviews.apache.org/r/53867/diff/
> >
> >
> > Testing
> > -------
> >
> >
> > Thanks,
> >
> > Hitesh Khamesra
> >
> >
>
>
> --
> "The World is a book, and those who do not travel read only a page." - St.
> Augustine.
>
>
>

Re: Review Request 53867: GEODE-2129 make pdxtype id random(not sequential)

Posted by Hitesh Khamesra <hi...@yahoo.com.INVALID>.

Thanks Brian for this info. Can you please elaborate a little bit more here.I am assuming here that import of pdxtypes registry(with the same dsid) ,  overrides the existing pdxTypes in the cluster?

      From: Brian Dunlap <bd...@gmail.com>
 To: dev@geode.apache.org 
Cc: Hitesh Khamesra <hi...@yahoo.com>; Hitesh Khamesra <hk...@pivotal.io>; Udo Kohlmeyer <uk...@gmail.com>; Bruce Schuchardt <bs...@pivotal.io>; geode <de...@geode.incubator.apache.org>
 Sent: Monday, November 21, 2016 9:14 AM
 Subject: Re: Review Request 53867: GEODE-2129 make pdxtype id random(not sequential)
   
Just FYI -

We've especially run into PDX collisions when we move region snapshots
between separate environments.
To get things working, we've needed to blow away all persisted regions
including the persisted pdx info.

It would be nice to improve snapshot portability.
For example load a single region from dev into QA vs. having to load
everything.

Thanks,
Brian -

On Mon, Nov 21, 2016 at 8:07 AM, Michael Stolz <ms...@pivotal.io> wrote:

> I think we need to be more forceful about requiring different distributed
> system id's. This idea of using hashcode seems like a debugging problem
> waiting to happen.
>
> --
> Mike Stolz
> Principal Engineer, GemFire Product Manager
> Mobile: 631-835-4771
>
> On Fri, Nov 18, 2016 at 6:39 PM, Hitesh Khamesra <
> hiteshk25@yahoo.com.invalid> wrote:
>
> > Before using hashcode for pdxType creation, we will check whether it is
> > available or not. If it's not available then, we will use next available
> > id. We have introduced this mechanism to harden the cases where the user
> > ends up having two clusters with the same dsid.        From: Vishal Rao <
> > vishalrao@gmail.com>
> >  To: dev@geode.apache.org; Hitesh Khamesra <hk...@pivotal.io>
> > Cc: Udo Kohlmeyer <uk...@gmail.com>; Bruce Schuchardt <
> > bschuchardt@pivotal.io>; geode <de...@geode.incubator.apache.org>
> >  Sent: Thursday, November 17, 2016 11:51 PM
> >  Subject: Re: Review Request 53867: GEODE-2129 make pdxtype id random(not
> > sequential)
> >
> > Apologies if this is noise, but wouldn't using the hash code actually
> > increase the likelihood of ID collisions?
> >
> > On Fri, Nov 18, 2016 at 6:11 AM, Hitesh Khamesra <hk...@pivotal.io>
> > wrote:
> >
> > >
> > > -----------------------------------------------------------
> > > This is an automatically generated e-mail. To reply, visit:
> > > https://reviews.apache.org/r/53867/
> > > -----------------------------------------------------------
> > >
> > > Review request for geode, Bruce Schuchardt and Udo Kohlmeyer.
> > >
> > >
> > > Repository: geode
> > >
> > >
> > > Description
> > > -------
> > >
> > > Right now pdxtype id has 4 bytes. Out of those 4 bytes, one byte
> reserved
> > > for distributed-system-id, this make sure type id generated from
> > different
> > > cluster has different id. For rest of the three bytes we just increment
> > > counter to create new pdxtype id. In the field, we have observed that
> > > sometimes this pdxType Id collides. One reason could be they end up
> > having
> > > same distributed-system-id for the different cluster.
> > > Thus to avoid a collision, we will be using hashcode of pdxType for
> three
> > > bytes of pdxType id. That will reduce the possibility of collision.
> > >
> > >
> > > Diffs
> > > -----
> > >
> > >  geode-core/src/main/java/org/apache/geode/pdx/internal/EnumId.java
> > > 5d399eb
> > >  geode-core/src/main/java/org/apache/geode/pdx/internal/PdxType.java
> > > b586f64
> > >  geode-core/src/main/java/org/apache/geode/pdx/internal/
> > PeerTypeRegistration.java
> > > 0226cca
> > >  geode-core/src/test/java/org/apache/geode/pdx/
> > PdxAttributesJUnitTest.java
> > > c45abce
> > >  geode-core/src/test/java/org/apache/geode/pdx/
> > PdxSerializableJUnitTest.java
> > > 5cd822c
> > >  geode-wan/src/test/java/org/apache/geode/internal/cache/
> > wan/WANTestBase.java
> > > f9c18ec
> > >  geode-wan/src/test/java/org/apache/geode/internal/cache/
> > > wan/misc/PDXNewWanDUnitTest.java f1e8f42
> > >
> > > Diff: https://reviews.apache.org/r/53867/diff/
> > >
> > >
> > > Testing
> > > -------
> > >
> > >
> > > Thanks,
> > >
> > > Hitesh Khamesra
> > >
> > >
> >
> >
> > --
> > "The World is a book, and those who do not travel read only a page." -
> St.
> > Augustine.
> >
> >
> >
>

Re: Review Request 53867: GEODE-2129 make pdxtype id random(not sequential)

Posted by Brian Dunlap <bd...@gmail.com>.

Just FYI -

We've especially run into PDX collisions when we move region snapshots
between separate environments.
To get things working, we've needed to blow away all persisted regions
including the persisted pdx info.

It would be nice to improve snapshot portability.
For example load a single region from dev into QA vs. having to load
everything.

Thanks,
Brian -

On Mon, Nov 21, 2016 at 8:07 AM, Michael Stolz <ms...@pivotal.io> wrote:

> I think we need to be more forceful about requiring different distributed
> system id's. This idea of using hashcode seems like a debugging problem
> waiting to happen.
>
> --
> Mike Stolz
> Principal Engineer, GemFire Product Manager
> Mobile: 631-835-4771
>
> On Fri, Nov 18, 2016 at 6:39 PM, Hitesh Khamesra <
> hiteshk25@yahoo.com.invalid> wrote:
>
> > Before using hashcode for pdxType creation, we will check whether it is
> > available or not. If it's not available then, we will use next available
> > id. We have introduced this mechanism to harden the cases where the user
> > ends up having two clusters with the same dsid.        From: Vishal Rao <
> > vishalrao@gmail.com>
> >  To: dev@geode.apache.org; Hitesh Khamesra <hk...@pivotal.io>
> > Cc: Udo Kohlmeyer <uk...@gmail.com>; Bruce Schuchardt <
> > bschuchardt@pivotal.io>; geode <de...@geode.incubator.apache.org>
> >  Sent: Thursday, November 17, 2016 11:51 PM
> >  Subject: Re: Review Request 53867: GEODE-2129 make pdxtype id random(not
> > sequential)
> >
> > Apologies if this is noise, but wouldn't using the hash code actually
> > increase the likelihood of ID collisions?
> >
> > On Fri, Nov 18, 2016 at 6:11 AM, Hitesh Khamesra <hk...@pivotal.io>
> > wrote:
> >
> > >
> > > -----------------------------------------------------------
> > > This is an automatically generated e-mail. To reply, visit:
> > > https://reviews.apache.org/r/53867/
> > > -----------------------------------------------------------
> > >
> > > Review request for geode, Bruce Schuchardt and Udo Kohlmeyer.
> > >
> > >
> > > Repository: geode
> > >
> > >
> > > Description
> > > -------
> > >
> > > Right now pdxtype id has 4 bytes. Out of those 4 bytes, one byte
> reserved
> > > for distributed-system-id, this make sure type id generated from
> > different
> > > cluster has different id. For rest of the three bytes we just increment
> > > counter to create new pdxtype id. In the field, we have observed that
> > > sometimes this pdxType Id collides. One reason could be they end up
> > having
> > > same distributed-system-id for the different cluster.
> > > Thus to avoid a collision, we will be using hashcode of pdxType for
> three
> > > bytes of pdxType id. That will reduce the possibility of collision.
> > >
> > >
> > > Diffs
> > > -----
> > >
> > >  geode-core/src/main/java/org/apache/geode/pdx/internal/EnumId.java
> > > 5d399eb
> > >  geode-core/src/main/java/org/apache/geode/pdx/internal/PdxType.java
> > > b586f64
> > >  geode-core/src/main/java/org/apache/geode/pdx/internal/
> > PeerTypeRegistration.java
> > > 0226cca
> > >  geode-core/src/test/java/org/apache/geode/pdx/
> > PdxAttributesJUnitTest.java
> > > c45abce
> > >  geode-core/src/test/java/org/apache/geode/pdx/
> > PdxSerializableJUnitTest.java
> > > 5cd822c
> > >  geode-wan/src/test/java/org/apache/geode/internal/cache/
> > wan/WANTestBase.java
> > > f9c18ec
> > >  geode-wan/src/test/java/org/apache/geode/internal/cache/
> > > wan/misc/PDXNewWanDUnitTest.java f1e8f42
> > >
> > > Diff: https://reviews.apache.org/r/53867/diff/
> > >
> > >
> > > Testing
> > > -------
> > >
> > >
> > > Thanks,
> > >
> > > Hitesh Khamesra
> > >
> > >
> >
> >
> > --
> > "The World is a book, and those who do not travel read only a page." -
> St.
> > Augustine.
> >
> >
> >
>

Re: Review Request 53867: GEODE-2129 make pdxtype id random(not sequential)

Posted by Michael Stolz <ms...@pivotal.io>.

I think we need to be more forceful about requiring different distributed
system id's. This idea of using hashcode seems like a debugging problem
waiting to happen.

--
Mike Stolz
Principal Engineer, GemFire Product Manager
Mobile: 631-835-4771

On Fri, Nov 18, 2016 at 6:39 PM, Hitesh Khamesra <
hiteshk25@yahoo.com.invalid> wrote:

> Before using hashcode for pdxType creation, we will check whether it is
> available or not. If it's not available then, we will use next available
> id. We have introduced this mechanism to harden the cases where the user
> ends up having two clusters with the same dsid.        From: Vishal Rao <
> vishalrao@gmail.com>
>  To: dev@geode.apache.org; Hitesh Khamesra <hk...@pivotal.io>
> Cc: Udo Kohlmeyer <uk...@gmail.com>; Bruce Schuchardt <
> bschuchardt@pivotal.io>; geode <de...@geode.incubator.apache.org>
>  Sent: Thursday, November 17, 2016 11:51 PM
>  Subject: Re: Review Request 53867: GEODE-2129 make pdxtype id random(not
> sequential)
>
> Apologies if this is noise, but wouldn't using the hash code actually
> increase the likelihood of ID collisions?
>
> On Fri, Nov 18, 2016 at 6:11 AM, Hitesh Khamesra <hk...@pivotal.io>
> wrote:
>
> >
> > -----------------------------------------------------------
> > This is an automatically generated e-mail. To reply, visit:
> > https://reviews.apache.org/r/53867/
> > -----------------------------------------------------------
> >
> > Review request for geode, Bruce Schuchardt and Udo Kohlmeyer.
> >
> >
> > Repository: geode
> >
> >
> > Description
> > -------
> >
> > Right now pdxtype id has 4 bytes. Out of those 4 bytes, one byte reserved
> > for distributed-system-id, this make sure type id generated from
> different
> > cluster has different id. For rest of the three bytes we just increment
> > counter to create new pdxtype id. In the field, we have observed that
> > sometimes this pdxType Id collides. One reason could be they end up
> having
> > same distributed-system-id for the different cluster.
> > Thus to avoid a collision, we will be using hashcode of pdxType for three
> > bytes of pdxType id. That will reduce the possibility of collision.
> >
> >
> > Diffs
> > -----
> >
> >  geode-core/src/main/java/org/apache/geode/pdx/internal/EnumId.java
> > 5d399eb
> >  geode-core/src/main/java/org/apache/geode/pdx/internal/PdxType.java
> > b586f64
> >  geode-core/src/main/java/org/apache/geode/pdx/internal/
> PeerTypeRegistration.java
> > 0226cca
> >  geode-core/src/test/java/org/apache/geode/pdx/
> PdxAttributesJUnitTest.java
> > c45abce
> >  geode-core/src/test/java/org/apache/geode/pdx/
> PdxSerializableJUnitTest.java
> > 5cd822c
> >  geode-wan/src/test/java/org/apache/geode/internal/cache/
> wan/WANTestBase.java
> > f9c18ec
> >  geode-wan/src/test/java/org/apache/geode/internal/cache/
> > wan/misc/PDXNewWanDUnitTest.java f1e8f42
> >
> > Diff: https://reviews.apache.org/r/53867/diff/
> >
> >
> > Testing
> > -------
> >
> >
> > Thanks,
> >
> > Hitesh Khamesra
> >
> >
>
>
> --
> "The World is a book, and those who do not travel read only a page." - St.
> Augustine.
>
>
>

Re: Review Request 53867: GEODE-2129 make pdxtype id random(not sequential)

Posted by Hitesh Khamesra <hi...@yahoo.com.INVALID>.

Before using hashcode for pdxType creation, we will check whether it is available or not. If it's not available then, we will use next available id. We have introduced this mechanism to harden the cases where the user ends up having two clusters with the same dsid.        From: Vishal Rao <vi...@gmail.com>
 To: dev@geode.apache.org; Hitesh Khamesra <hk...@pivotal.io> 
Cc: Udo Kohlmeyer <uk...@gmail.com>; Bruce Schuchardt <bs...@pivotal.io>; geode <de...@geode.incubator.apache.org>
 Sent: Thursday, November 17, 2016 11:51 PM
 Subject: Re: Review Request 53867: GEODE-2129 make pdxtype id random(not sequential)

Apologies if this is noise, but wouldn't using the hash code actually
increase the likelihood of ID collisions?

On Fri, Nov 18, 2016 at 6:11 AM, Hitesh Khamesra <hk...@pivotal.io>
wrote:

>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53867/
> -----------------------------------------------------------
>
> Review request for geode, Bruce Schuchardt and Udo Kohlmeyer.
>
>
> Repository: geode
>
>
> Description
> -------
>
> Right now pdxtype id has 4 bytes. Out of those 4 bytes, one byte reserved
> for distributed-system-id, this make sure type id generated from different
> cluster has different id. For rest of the three bytes we just increment
> counter to create new pdxtype id. In the field, we have observed that
> sometimes this pdxType Id collides. One reason could be they end up having
> same distributed-system-id for the different cluster.
> Thus to avoid a collision, we will be using hashcode of pdxType for three
> bytes of pdxType id. That will reduce the possibility of collision.
>
>
> Diffs
> -----
>
>  geode-core/src/main/java/org/apache/geode/pdx/internal/EnumId.java
> 5d399eb
>  geode-core/src/main/java/org/apache/geode/pdx/internal/PdxType.java
> b586f64
>  geode-core/src/main/java/org/apache/geode/pdx/internal/PeerTypeRegistration.java
> 0226cca
>  geode-core/src/test/java/org/apache/geode/pdx/PdxAttributesJUnitTest.java
> c45abce
>  geode-core/src/test/java/org/apache/geode/pdx/PdxSerializableJUnitTest.java
> 5cd822c
>  geode-wan/src/test/java/org/apache/geode/internal/cache/wan/WANTestBase.java
> f9c18ec
>  geode-wan/src/test/java/org/apache/geode/internal/cache/
> wan/misc/PDXNewWanDUnitTest.java f1e8f42
>
> Diff: https://reviews.apache.org/r/53867/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hitesh Khamesra
>
>

-- 
"The World is a book, and those who do not travel read only a page." - St.
Augustine.

Re: Review Request 53867: GEODE-2129 make pdxtype id random(not sequential)

Posted by Vishal Rao <vi...@gmail.com>.

Apologies if this is noise, but wouldn't using the hash code actually
increase the likelihood of ID collisions?

On Fri, Nov 18, 2016 at 6:11 AM, Hitesh Khamesra <hk...@pivotal.io>
wrote:

>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53867/
> -----------------------------------------------------------
>
> Review request for geode, Bruce Schuchardt and Udo Kohlmeyer.
>
>
> Repository: geode
>
>
> Description
> -------
>
> Right now pdxtype id has 4 bytes. Out of those 4 bytes, one byte reserved
> for distributed-system-id, this make sure type id generated from different
> cluster has different id. For rest of the three bytes we just increment
> counter to create new pdxtype id. In the field, we have observed that
> sometimes this pdxType Id collides. One reason could be they end up having
> same distributed-system-id for the different cluster.
> Thus to avoid a collision, we will be using hashcode of pdxType for three
> bytes of pdxType id. That will reduce the possibility of collision.
>
>
> Diffs
> -----
>
>   geode-core/src/main/java/org/apache/geode/pdx/internal/EnumId.java
> 5d399eb
>   geode-core/src/main/java/org/apache/geode/pdx/internal/PdxType.java
> b586f64
>   geode-core/src/main/java/org/apache/geode/pdx/internal/PeerTypeRegistration.java
> 0226cca
>   geode-core/src/test/java/org/apache/geode/pdx/PdxAttributesJUnitTest.java
> c45abce
>   geode-core/src/test/java/org/apache/geode/pdx/PdxSerializableJUnitTest.java
> 5cd822c
>   geode-wan/src/test/java/org/apache/geode/internal/cache/wan/WANTestBase.java
> f9c18ec
>   geode-wan/src/test/java/org/apache/geode/internal/cache/
> wan/misc/PDXNewWanDUnitTest.java f1e8f42
>
> Diff: https://reviews.apache.org/r/53867/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hitesh Khamesra
>
>


-- 
"The World is a book, and those who do not travel read only a page." - St.
Augustine.

Re: Review Request 53867: GEODE-2129 make pdxtype id random(not sequential)

Posted by Bruce Schuchardt <bs...@pivotal.io>.

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




geode-core/src/main/java/org/apache/geode/pdx/internal/PeerTypeRegistration.java (line 284)
<https://reviews.apache.org/r/53867/#comment226524>

    Shouldn't this be using newEnumId?  I think you should delete nextEnumId as it caused confusion here and it's not needed anymore.



geode-core/src/main/java/org/apache/geode/pdx/internal/PeerTypeRegistration.java (line 562)
<https://reviews.apache.org/r/53867/#comment226531>

    The check for totalPdxTypeIdInDS == maxTypeId should be moved to this method and you should delete the instance variable totalPdxTypeIdInDS.  Otherwise you've introduced a weird dependency between the two methods.  Rename this method to indicate it is doing this check.



geode-core/src/main/java/org/apache/geode/pdx/internal/PeerTypeRegistration.java (line 596)
<https://reviews.apache.org/r/53867/#comment226532>

    Same comment here - get rid of this.totalEnumIdInDS and throw the exception in this method.
    
    If it shouldn't perform the check in some code paths you could parameterize the method to turn the check on/off.


- Bruce Schuchardt


On Nov. 18, 2016, 12:41 a.m., Hitesh Khamesra wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53867/
> -----------------------------------------------------------
> 
> (Updated Nov. 18, 2016, 12:41 a.m.)
> 
> 
> Review request for geode, Bruce Schuchardt and Udo Kohlmeyer.
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> Right now pdxtype id has 4 bytes. Out of those 4 bytes, one byte reserved for distributed-system-id, this make sure type id generated from different cluster has different id. For rest of the three bytes we just increment counter to create new pdxtype id. In the field, we have observed that sometimes this pdxType Id collides. One reason could be they end up having same distributed-system-id for the different cluster.
> Thus to avoid a collision, we will be using hashcode of pdxType for three bytes of pdxType id. That will reduce the possibility of collision.
> 
> 
> Diffs
> -----
> 
>   geode-core/src/main/java/org/apache/geode/pdx/internal/EnumId.java 5d399eb 
>   geode-core/src/main/java/org/apache/geode/pdx/internal/PdxType.java b586f64 
>   geode-core/src/main/java/org/apache/geode/pdx/internal/PeerTypeRegistration.java 0226cca 
>   geode-core/src/test/java/org/apache/geode/pdx/PdxAttributesJUnitTest.java c45abce 
>   geode-core/src/test/java/org/apache/geode/pdx/PdxSerializableJUnitTest.java 5cd822c 
>   geode-wan/src/test/java/org/apache/geode/internal/cache/wan/WANTestBase.java f9c18ec 
>   geode-wan/src/test/java/org/apache/geode/internal/cache/wan/misc/PDXNewWanDUnitTest.java f1e8f42 
> 
> Diff: https://reviews.apache.org/r/53867/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Hitesh Khamesra
> 
>