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