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