----------------------------------------------------------- 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 > >
