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 2017/01/10 19:36:22 UTC
Review Request 55389: GEODE-2282 Provide ability to sort field while
creating pdxType for jSON document
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55389/
-----------------------------------------------------------
Review request for geode, Bruce Schuchardt and Udo Kohlmeyer.
Repository: geode
Description
-------
Added ability to sort json field while convertsing json to pdx. It will help us reduce number of pdxType id, when document has fields in duifferent order. One can enable this feature using java system property gemfire.sort-json-field-names
Diffs
-----
geode-core/src/main/java/org/apache/geode/pdx/JSONFormatter.java a96e111
geode-core/src/main/java/org/apache/geode/pdx/internal/json/JSONToPdxMapper.java PRE-CREATION
geode-core/src/main/java/org/apache/geode/pdx/internal/json/PdxInstanceHelper.java a91fbd4
geode-core/src/main/java/org/apache/geode/pdx/internal/json/PdxInstanceSortedHelper.java PRE-CREATION
geode-core/src/main/java/org/apache/geode/pdx/internal/json/PdxListHelper.java caf0dfc
geode-core/src/test/java/org/apache/geode/pdx/JSONFormatterJUnitTest.java cbe350f
geode-core/src/test/java/org/apache/geode/pdx/JSONPdxClientServerDUnitTest.java ae17837
Diff: https://reviews.apache.org/r/55389/diff/
Testing
-------
Thanks,
Hitesh Khamesra
Re: Review Request 55389: GEODE-2282 Provide ability to sort field
while creating pdxType for jSON document
Posted by Udo Kohlmeyer <uk...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55389/#review161447
-----------------------------------------------------------
geode-core/src/main/java/org/apache/geode/pdx/internal/json/PdxInstanceSortedHelper.java (line 97)
<https://reviews.apache.org/r/55389/#comment232704>
maybe a very niche "improvement" but could we be "smart" and order the fields in alphabetical AND maybe data types? In ReflectionPdxSerializer we try and do fixed length fields before we do variable length. Might be a "nice" improvement.
- Udo Kohlmeyer
On Jan. 10, 2017, 7:36 p.m., Hitesh Khamesra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55389/
> -----------------------------------------------------------
>
> (Updated Jan. 10, 2017, 7:36 p.m.)
>
>
> Review request for geode, Bruce Schuchardt and Udo Kohlmeyer.
>
>
> Repository: geode
>
>
> Description
> -------
>
> Added ability to sort json field while convertsing json to pdx. It will help us reduce number of pdxType id, when document has fields in duifferent order. One can enable this feature using java system property gemfire.sort-json-field-names
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/pdx/JSONFormatter.java a96e111
> geode-core/src/main/java/org/apache/geode/pdx/internal/json/JSONToPdxMapper.java PRE-CREATION
> geode-core/src/main/java/org/apache/geode/pdx/internal/json/PdxInstanceHelper.java a91fbd4
> geode-core/src/main/java/org/apache/geode/pdx/internal/json/PdxInstanceSortedHelper.java PRE-CREATION
> geode-core/src/main/java/org/apache/geode/pdx/internal/json/PdxListHelper.java caf0dfc
> geode-core/src/test/java/org/apache/geode/pdx/JSONFormatterJUnitTest.java cbe350f
> geode-core/src/test/java/org/apache/geode/pdx/JSONPdxClientServerDUnitTest.java ae17837
>
> Diff: https://reviews.apache.org/r/55389/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hitesh Khamesra
>
>
Re: Review Request 55389: GEODE-2282 Provide ability to sort field
while creating pdxType for jSON document
Posted by Bruce Schuchardt <bs...@pivotal.io>.
> On Jan. 10, 2017, 9:44 p.m., Bruce Schuchardt wrote:
> > geode-core/src/main/java/org/apache/geode/pdx/JSONFormatter.java, line 104
> > <https://reviews.apache.org/r/55389/diff/1/?file=1601773#file1601773line104>
> >
> > I don't think you should have this static variable. You should look up the property each time we're going to parse a document.
> >
> > If you keep the variable either rename it so it doesn't look like a constant or make it final. In either case it will need a good javadoc since it's public.
>
> Hitesh Khamesra wrote:
> Do we have any perf concern if we check property each time? Do we have this sort of thing somewhere else in geode? If there is no concern then I would prefer to do this. Otherwise we need to comeup some mechanism to test this property.
>
> Udo Kohlmeyer wrote:
> I agree, since the previous of the JSONFormatter does not store state, this should not either. Every JSON document could be different. This might be up for an improvement if we can get to a framework that can map incoming/exporting data.
I don't think that a simple map lookup using a String key is going to impact performance if it's only done once for each document that's parsed.
- Bruce
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55389/#review161123
-----------------------------------------------------------
On Jan. 10, 2017, 7:36 p.m., Hitesh Khamesra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55389/
> -----------------------------------------------------------
>
> (Updated Jan. 10, 2017, 7:36 p.m.)
>
>
> Review request for geode, Bruce Schuchardt and Udo Kohlmeyer.
>
>
> Repository: geode
>
>
> Description
> -------
>
> Added ability to sort json field while convertsing json to pdx. It will help us reduce number of pdxType id, when document has fields in duifferent order. One can enable this feature using java system property gemfire.sort-json-field-names
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/pdx/JSONFormatter.java a96e111
> geode-core/src/main/java/org/apache/geode/pdx/internal/json/JSONToPdxMapper.java PRE-CREATION
> geode-core/src/main/java/org/apache/geode/pdx/internal/json/PdxInstanceHelper.java a91fbd4
> geode-core/src/main/java/org/apache/geode/pdx/internal/json/PdxInstanceSortedHelper.java PRE-CREATION
> geode-core/src/main/java/org/apache/geode/pdx/internal/json/PdxListHelper.java caf0dfc
> geode-core/src/test/java/org/apache/geode/pdx/JSONFormatterJUnitTest.java cbe350f
> geode-core/src/test/java/org/apache/geode/pdx/JSONPdxClientServerDUnitTest.java ae17837
>
> Diff: https://reviews.apache.org/r/55389/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hitesh Khamesra
>
>
Re: Review Request 55389: GEODE-2282 Provide ability to sort field
while creating pdxType for jSON document
Posted by Udo Kohlmeyer <uk...@gmail.com>.
> On Jan. 10, 2017, 9:44 p.m., Bruce Schuchardt wrote:
> > geode-core/src/main/java/org/apache/geode/pdx/JSONFormatter.java, line 104
> > <https://reviews.apache.org/r/55389/diff/1/?file=1601773#file1601773line104>
> >
> > I don't think you should have this static variable. You should look up the property each time we're going to parse a document.
> >
> > If you keep the variable either rename it so it doesn't look like a constant or make it final. In either case it will need a good javadoc since it's public.
>
> Hitesh Khamesra wrote:
> Do we have any perf concern if we check property each time? Do we have this sort of thing somewhere else in geode? If there is no concern then I would prefer to do this. Otherwise we need to comeup some mechanism to test this property.
I agree, since the previous of the JSONFormatter does not store state, this should not either. Every JSON document could be different. This might be up for an improvement if we can get to a framework that can map incoming/exporting data.
- Udo
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55389/#review161123
-----------------------------------------------------------
On Jan. 10, 2017, 7:36 p.m., Hitesh Khamesra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55389/
> -----------------------------------------------------------
>
> (Updated Jan. 10, 2017, 7:36 p.m.)
>
>
> Review request for geode, Bruce Schuchardt and Udo Kohlmeyer.
>
>
> Repository: geode
>
>
> Description
> -------
>
> Added ability to sort json field while convertsing json to pdx. It will help us reduce number of pdxType id, when document has fields in duifferent order. One can enable this feature using java system property gemfire.sort-json-field-names
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/pdx/JSONFormatter.java a96e111
> geode-core/src/main/java/org/apache/geode/pdx/internal/json/JSONToPdxMapper.java PRE-CREATION
> geode-core/src/main/java/org/apache/geode/pdx/internal/json/PdxInstanceHelper.java a91fbd4
> geode-core/src/main/java/org/apache/geode/pdx/internal/json/PdxInstanceSortedHelper.java PRE-CREATION
> geode-core/src/main/java/org/apache/geode/pdx/internal/json/PdxListHelper.java caf0dfc
> geode-core/src/test/java/org/apache/geode/pdx/JSONFormatterJUnitTest.java cbe350f
> geode-core/src/test/java/org/apache/geode/pdx/JSONPdxClientServerDUnitTest.java ae17837
>
> Diff: https://reviews.apache.org/r/55389/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hitesh Khamesra
>
>
Re: Review Request 55389: GEODE-2282 Provide ability to sort field
while creating pdxType for jSON document
Posted by Hitesh Khamesra <hk...@pivotal.io>.
> On Jan. 10, 2017, 9:44 p.m., Bruce Schuchardt wrote:
> > geode-core/src/main/java/org/apache/geode/pdx/JSONFormatter.java, line 104
> > <https://reviews.apache.org/r/55389/diff/1/?file=1601773#file1601773line104>
> >
> > I don't think you should have this static variable. You should look up the property each time we're going to parse a document.
> >
> > If you keep the variable either rename it so it doesn't look like a constant or make it final. In either case it will need a good javadoc since it's public.
>
> Hitesh Khamesra wrote:
> Do we have any perf concern if we check property each time? Do we have this sort of thing somewhere else in geode? If there is no concern then I would prefer to do this. Otherwise we need to comeup some mechanism to test this property.
>
> Udo Kohlmeyer wrote:
> I agree, since the previous of the JSONFormatter does not store state, this should not either. Every JSON document could be different. This might be up for an improvement if we can get to a framework that can map incoming/exporting data.
>
> Bruce Schuchardt wrote:
> I don't think that a simple map lookup using a String key is going to impact performance if it's only done once for each document that's parsed.
Agree. this will make life simpler :)
- Hitesh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55389/#review161123
-----------------------------------------------------------
On Jan. 10, 2017, 7:36 p.m., Hitesh Khamesra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55389/
> -----------------------------------------------------------
>
> (Updated Jan. 10, 2017, 7:36 p.m.)
>
>
> Review request for geode, Bruce Schuchardt and Udo Kohlmeyer.
>
>
> Repository: geode
>
>
> Description
> -------
>
> Added ability to sort json field while convertsing json to pdx. It will help us reduce number of pdxType id, when document has fields in duifferent order. One can enable this feature using java system property gemfire.sort-json-field-names
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/pdx/JSONFormatter.java a96e111
> geode-core/src/main/java/org/apache/geode/pdx/internal/json/JSONToPdxMapper.java PRE-CREATION
> geode-core/src/main/java/org/apache/geode/pdx/internal/json/PdxInstanceHelper.java a91fbd4
> geode-core/src/main/java/org/apache/geode/pdx/internal/json/PdxInstanceSortedHelper.java PRE-CREATION
> geode-core/src/main/java/org/apache/geode/pdx/internal/json/PdxListHelper.java caf0dfc
> geode-core/src/test/java/org/apache/geode/pdx/JSONFormatterJUnitTest.java cbe350f
> geode-core/src/test/java/org/apache/geode/pdx/JSONPdxClientServerDUnitTest.java ae17837
>
> Diff: https://reviews.apache.org/r/55389/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hitesh Khamesra
>
>
Re: Review Request 55389: GEODE-2282 Provide ability to sort field
while creating pdxType for jSON document
Posted by Hitesh Khamesra <hk...@pivotal.io>.
> On Jan. 10, 2017, 9:44 p.m., Bruce Schuchardt wrote:
> > We shouldn't have two disjoint PdxInstanceHelper classes when the only difference in behavior is the sorting of fields. This duplication makes the code harder to maintain. I think you should get rid of the old helper class and change the new one to sort or not sort the fields before generating PDX artifacts. Or have a subclass of the new helper that performs the sort before artifacts are generated.
Both have totally different implimentation. One is using PDxInstancefactory in each api and other is collecting fields. If I will combine then there will be "if" in each api. That's why I introduce interface to avoid that.
> On Jan. 10, 2017, 9:44 p.m., Bruce Schuchardt wrote:
> > geode-core/src/main/java/org/apache/geode/pdx/JSONFormatter.java, line 101
> > <https://reviews.apache.org/r/55389/diff/1/?file=1601773#file1601773line101>
> >
> > As a public static in a public class this variable must have a javadoc.
I will update that.
> On Jan. 10, 2017, 9:44 p.m., Bruce Schuchardt wrote:
> > geode-core/src/main/java/org/apache/geode/pdx/JSONFormatter.java, line 104
> > <https://reviews.apache.org/r/55389/diff/1/?file=1601773#file1601773line104>
> >
> > I don't think you should have this static variable. You should look up the property each time we're going to parse a document.
> >
> > If you keep the variable either rename it so it doesn't look like a constant or make it final. In either case it will need a good javadoc since it's public.
Do we have any perf concern if we check property each time? Do we have this sort of thing somewhere else in geode? If there is no concern then I would prefer to do this. Otherwise we need to comeup some mechanism to test this property.
- Hitesh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55389/#review161123
-----------------------------------------------------------
On Jan. 10, 2017, 7:36 p.m., Hitesh Khamesra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55389/
> -----------------------------------------------------------
>
> (Updated Jan. 10, 2017, 7:36 p.m.)
>
>
> Review request for geode, Bruce Schuchardt and Udo Kohlmeyer.
>
>
> Repository: geode
>
>
> Description
> -------
>
> Added ability to sort json field while convertsing json to pdx. It will help us reduce number of pdxType id, when document has fields in duifferent order. One can enable this feature using java system property gemfire.sort-json-field-names
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/pdx/JSONFormatter.java a96e111
> geode-core/src/main/java/org/apache/geode/pdx/internal/json/JSONToPdxMapper.java PRE-CREATION
> geode-core/src/main/java/org/apache/geode/pdx/internal/json/PdxInstanceHelper.java a91fbd4
> geode-core/src/main/java/org/apache/geode/pdx/internal/json/PdxInstanceSortedHelper.java PRE-CREATION
> geode-core/src/main/java/org/apache/geode/pdx/internal/json/PdxListHelper.java caf0dfc
> geode-core/src/test/java/org/apache/geode/pdx/JSONFormatterJUnitTest.java cbe350f
> geode-core/src/test/java/org/apache/geode/pdx/JSONPdxClientServerDUnitTest.java ae17837
>
> Diff: https://reviews.apache.org/r/55389/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hitesh Khamesra
>
>
Re: Review Request 55389: GEODE-2282 Provide ability to sort field
while creating pdxType for jSON document
Posted by Bruce Schuchardt <bs...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55389/#review161123
-----------------------------------------------------------
We shouldn't have two disjoint PdxInstanceHelper classes when the only difference in behavior is the sorting of fields. This duplication makes the code harder to maintain. I think you should get rid of the old helper class and change the new one to sort or not sort the fields before generating PDX artifacts. Or have a subclass of the new helper that performs the sort before artifacts are generated.
geode-core/src/main/java/org/apache/geode/pdx/JSONFormatter.java (line 101)
<https://reviews.apache.org/r/55389/#comment232362>
As a public static in a public class this variable must have a javadoc.
geode-core/src/main/java/org/apache/geode/pdx/JSONFormatter.java (line 104)
<https://reviews.apache.org/r/55389/#comment232364>
I don't think you should have this static variable. You should look up the property each time we're going to parse a document.
If you keep the variable either rename it so it doesn't look like a constant or make it final. In either case it will need a good javadoc since it's public.
geode-core/src/test/java/org/apache/geode/pdx/JSONFormatterJUnitTest.java (line 284)
<https://reviews.apache.org/r/55389/#comment232363>
If the JsonFormatter class is touched in some other test prior to running this test your system property will have no effect.
Should JsonFormatter look up the property each time it's going to parse a document instead of caching the property in a static variable?
- Bruce Schuchardt
On Jan. 10, 2017, 7:36 p.m., Hitesh Khamesra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55389/
> -----------------------------------------------------------
>
> (Updated Jan. 10, 2017, 7:36 p.m.)
>
>
> Review request for geode, Bruce Schuchardt and Udo Kohlmeyer.
>
>
> Repository: geode
>
>
> Description
> -------
>
> Added ability to sort json field while convertsing json to pdx. It will help us reduce number of pdxType id, when document has fields in duifferent order. One can enable this feature using java system property gemfire.sort-json-field-names
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/pdx/JSONFormatter.java a96e111
> geode-core/src/main/java/org/apache/geode/pdx/internal/json/JSONToPdxMapper.java PRE-CREATION
> geode-core/src/main/java/org/apache/geode/pdx/internal/json/PdxInstanceHelper.java a91fbd4
> geode-core/src/main/java/org/apache/geode/pdx/internal/json/PdxInstanceSortedHelper.java PRE-CREATION
> geode-core/src/main/java/org/apache/geode/pdx/internal/json/PdxListHelper.java caf0dfc
> geode-core/src/test/java/org/apache/geode/pdx/JSONFormatterJUnitTest.java cbe350f
> geode-core/src/test/java/org/apache/geode/pdx/JSONPdxClientServerDUnitTest.java ae17837
>
> Diff: https://reviews.apache.org/r/55389/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hitesh Khamesra
>
>
Re: Review Request 55389: GEODE-2282 Provide ability to sort field
while creating pdxType for jSON document
Posted by Hitesh Khamesra <hk...@pivotal.io>.
> On Jan. 12, 2017, 10:55 p.m., Udo Kohlmeyer wrote:
> > geode-core/src/main/java/org/apache/geode/pdx/JSONFormatter.java, lines 194-197
> > <https://reviews.apache.org/r/55389/diff/1/?file=1601773#file1601773line194>
> >
> > Maybe this could be a little "smarter". Maybe you have a DataMapperFactory or PDXDataMapperFactory. Then this could return you the correct dataMapper.
> > This is also a pattern that would allow us to be more extendable.
Udo: lets talk on all the improvements and file the other tciekt for it. Thanks.
- Hitesh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55389/#review161442
-----------------------------------------------------------
On Jan. 10, 2017, 7:36 p.m., Hitesh Khamesra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55389/
> -----------------------------------------------------------
>
> (Updated Jan. 10, 2017, 7:36 p.m.)
>
>
> Review request for geode, Bruce Schuchardt and Udo Kohlmeyer.
>
>
> Repository: geode
>
>
> Description
> -------
>
> Added ability to sort json field while convertsing json to pdx. It will help us reduce number of pdxType id, when document has fields in duifferent order. One can enable this feature using java system property gemfire.sort-json-field-names
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/pdx/JSONFormatter.java a96e111
> geode-core/src/main/java/org/apache/geode/pdx/internal/json/JSONToPdxMapper.java PRE-CREATION
> geode-core/src/main/java/org/apache/geode/pdx/internal/json/PdxInstanceHelper.java a91fbd4
> geode-core/src/main/java/org/apache/geode/pdx/internal/json/PdxInstanceSortedHelper.java PRE-CREATION
> geode-core/src/main/java/org/apache/geode/pdx/internal/json/PdxListHelper.java caf0dfc
> geode-core/src/test/java/org/apache/geode/pdx/JSONFormatterJUnitTest.java cbe350f
> geode-core/src/test/java/org/apache/geode/pdx/JSONPdxClientServerDUnitTest.java ae17837
>
> Diff: https://reviews.apache.org/r/55389/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hitesh Khamesra
>
>
Re: Review Request 55389: GEODE-2282 Provide ability to sort field
while creating pdxType for jSON document
Posted by Udo Kohlmeyer <uk...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55389/#review161442
-----------------------------------------------------------
geode-core/src/main/java/org/apache/geode/pdx/JSONFormatter.java (lines 101 - 102)
<https://reviews.apache.org/r/55389/#comment232689>
A more general comment, but I think we should come up with a property namespace hierarchy that will make it clear where the "belong" to. maybe something like GEODE/GEMFIRE_PREFIX+"pdx.mapper.sort-json-field-names".
geode-core/src/main/java/org/apache/geode/pdx/JSONFormatter.java (lines 194 - 197)
<https://reviews.apache.org/r/55389/#comment232692>
Maybe this could be a little "smarter". Maybe you have a DataMapperFactory or PDXDataMapperFactory. Then this could return you the correct dataMapper.
This is also a pattern that would allow us to be more extendable.
geode-core/src/main/java/org/apache/geode/pdx/internal/json/PdxInstanceSortedHelper.java (lines 115 - 118)
<https://reviews.apache.org/r/55389/#comment232698>
The add****Field method all follow a simple pattern...
* log method and field information
* add field to a field list
Could we not get a generic method out of this and instead of "copy paste" the same logic over and over again... rather call the generic method:
insertField(parentMethodName,fieldName,value,FieldType)
geode-core/src/main/java/org/apache/geode/pdx/internal/json/PdxInstanceSortedHelper.java (lines 293 - 294)
<https://reviews.apache.org/r/55389/#comment232699>
Provide {} for the if statement. Did spotless not pick this up?
geode-core/src/main/java/org/apache/geode/pdx/internal/json/PdxInstanceSortedHelper.java (line 321)
<https://reviews.apache.org/r/55389/#comment232700>
Why would this be a static method?
- Udo Kohlmeyer
On Jan. 10, 2017, 7:36 p.m., Hitesh Khamesra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55389/
> -----------------------------------------------------------
>
> (Updated Jan. 10, 2017, 7:36 p.m.)
>
>
> Review request for geode, Bruce Schuchardt and Udo Kohlmeyer.
>
>
> Repository: geode
>
>
> Description
> -------
>
> Added ability to sort json field while convertsing json to pdx. It will help us reduce number of pdxType id, when document has fields in duifferent order. One can enable this feature using java system property gemfire.sort-json-field-names
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/pdx/JSONFormatter.java a96e111
> geode-core/src/main/java/org/apache/geode/pdx/internal/json/JSONToPdxMapper.java PRE-CREATION
> geode-core/src/main/java/org/apache/geode/pdx/internal/json/PdxInstanceHelper.java a91fbd4
> geode-core/src/main/java/org/apache/geode/pdx/internal/json/PdxInstanceSortedHelper.java PRE-CREATION
> geode-core/src/main/java/org/apache/geode/pdx/internal/json/PdxListHelper.java caf0dfc
> geode-core/src/test/java/org/apache/geode/pdx/JSONFormatterJUnitTest.java cbe350f
> geode-core/src/test/java/org/apache/geode/pdx/JSONPdxClientServerDUnitTest.java ae17837
>
> Diff: https://reviews.apache.org/r/55389/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hitesh Khamesra
>
>
Re: Review Request 55389: GEODE-2282 Provide ability to sort field
while creating pdxType for jSON document
Posted by Udo Kohlmeyer <uk...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55389/#review161449
-----------------------------------------------------------
geode-core/src/main/java/org/apache/geode/pdx/JSONFormatter.java (line 121)
<https://reviews.apache.org/r/55389/#comment232707>
Maybe as part of a refactor the Factory can be held onto. According to the docs they are reusable and thread-safe.
geode-core/src/main/java/org/apache/geode/pdx/JSONFormatter.java (line 144)
<https://reviews.apache.org/r/55389/#comment232709>
Maybe we can have a "getJSONParser(Object input)" method that would return a jsonParser. Then we can merge the two fromJson methods or at least not duplicate the internals.
- Udo Kohlmeyer
On Jan. 10, 2017, 7:36 p.m., Hitesh Khamesra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55389/
> -----------------------------------------------------------
>
> (Updated Jan. 10, 2017, 7:36 p.m.)
>
>
> Review request for geode, Bruce Schuchardt and Udo Kohlmeyer.
>
>
> Repository: geode
>
>
> Description
> -------
>
> Added ability to sort json field while convertsing json to pdx. It will help us reduce number of pdxType id, when document has fields in duifferent order. One can enable this feature using java system property gemfire.sort-json-field-names
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/pdx/JSONFormatter.java a96e111
> geode-core/src/main/java/org/apache/geode/pdx/internal/json/JSONToPdxMapper.java PRE-CREATION
> geode-core/src/main/java/org/apache/geode/pdx/internal/json/PdxInstanceHelper.java a91fbd4
> geode-core/src/main/java/org/apache/geode/pdx/internal/json/PdxInstanceSortedHelper.java PRE-CREATION
> geode-core/src/main/java/org/apache/geode/pdx/internal/json/PdxListHelper.java caf0dfc
> geode-core/src/test/java/org/apache/geode/pdx/JSONFormatterJUnitTest.java cbe350f
> geode-core/src/test/java/org/apache/geode/pdx/JSONPdxClientServerDUnitTest.java ae17837
>
> Diff: https://reviews.apache.org/r/55389/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hitesh Khamesra
>
>