> On Sept. 10, 2015, 7:49 a.m., Rajat Khandelwal wrote: > > lens-server-api/src/main/java/org/apache/lens/server/api/query/save/SavedQueryHelper.java, > > line 88 > > <https://reviews.apache.org/r/37934/diff/6/?file=1066451#file1066451line88> > > > > Just a thought here: `parameter.getDataType()` is converted to > > `ParameterDataTypeEncoder` and sample value is asked. perhaps both(data > > type and encoder) can be merged?
It was like that initially but we would ideally want to keep the user facing entities in lens-api. If we inline some methods into the models, it might have to throw exceptions. And in lens-api, LensException is not available > On Sept. 10, 2015, 7:49 a.m., Rajat Khandelwal wrote: > > lens-api/src/main/resources/lens-errors.conf, lines 105-108 > > <https://reviews.apache.org/r/37934/diff/6/?file=1066449#file1066449line105> > > > > Why do we need to remove it? There is an INVALID_XML_ERROR in LensCommonErrorCode which is best suited to this. > On Sept. 10, 2015, 7:49 a.m., Rajat Khandelwal wrote: > > lens-api/src/main/java/org/apache/lens/api/query/save/ListResponse.java, > > line 30 > > <https://reviews.apache.org/r/37934/diff/6/?file=1066442#file1066442line30> > > > > This response can be generified to anything that is supposed to return > > a list. > > > > Can we probably make this `ListResponse<T>` and use > > `ListResponse<SavedQuery>` wherever required in this patch. There is an issue when we try to use generics and marshalling. The default MoxYJsonProvider seem to run into a bug whenever it encounters generics (whenever accept header is JSON). Existing APIs like run which results a LensAPIResult<T> is also failing when the accept header is JSON. > On Sept. 10, 2015, 7:49 a.m., Rajat Khandelwal wrote: > > lens-api/src/main/java/org/apache/lens/api/query/save/SavedQuery.java, line > > 50 > > <https://reviews.apache.org/r/37934/diff/6/?file=1066448#file1066448line50> > > > > `Parameter` has `name`, `displayName` but not `description`. > > `SavedQuery` has `name`, `description` but not `displayName`. Should they > > both have all three? > > > > Thinking more on this, `XField` has all three, so maybe they can extend > > that. But if that happens, we'll be better off renaming `XField` to > > something that reveals it has 3 compulsary methods. display name for parameter is needed as the query cannot have pretty parameter names but description for the same would be too much too ask (@parameter level) In case of saved query, the name itself could be pretty. So display name is not needed here > On Sept. 10, 2015, 7:49 a.m., Rajat Khandelwal wrote: > > lens-server-api/src/main/java/org/apache/lens/server/api/query/save/SavedQueryHelper.java, > > line 97 > > <https://reviews.apache.org/r/37934/diff/6/?file=1066451#file1066451line97> > > > > why two values? This is generating sample value for collection. So two values would make more sense. > On Sept. 10, 2015, 7:49 a.m., Rajat Khandelwal wrote: > > lens-server-api/src/main/java/org/apache/lens/server/api/query/save/SavedQueryService.java, > > line 63 > > <https://reviews.apache.org/r/37934/diff/6/?file=1066452#file1066452line63> > > > > Instead of `long`, can we re-use `QueryHandle`? The IDs are generated by the underlying store which I feel is better instead of generating them at server? Your thoughts? > On Sept. 10, 2015, 7:49 a.m., Rajat Khandelwal wrote: > > lens-server-api/src/main/java/org/apache/lens/server/api/query/save/SavedQueryHelper.java, > > lines 132-134 > > <https://reviews.apache.org/r/37934/diff/6/?file=1066451#file1066451line132> > > > > Instead of validating outside of `parameter`, creation of `Parameter` > > Object itself should be valid. As in these checks should be at the > > constructor level. If I have to keep the object immutable, I need to have XML entities for each model entity. > On Sept. 10, 2015, 7:49 a.m., Rajat Khandelwal wrote: > > lens-api/src/main/java/org/apache/lens/api/query/save/Parameter.java, line > > 42 > > <https://reviews.apache.org/r/37934/diff/6/?file=1066443#file1066443line42> > > > > final? I need getters and setters for jaxb to do marshalling and un marshalling (?). If I have to keep the object immutable, I need to have XML entities for each model entity. Can I do these changes ? > On Sept. 10, 2015, 7:49 a.m., Rajat Khandelwal wrote: > > lens-server-api/src/main/java/org/apache/lens/server/api/query/save/SavedQueryHelper.java, > > lines 112-113 > > <https://reviews.apache.org/r/37934/diff/6/?file=1066451#file1066451line112> > > > > Validations should be on constructor level. Again this is possible if I can have user facing entities for every model entity I have and let JAXB do the marsh/unmarsh for user facing entities. I can convert user facing entity to model entity internally. Should I proceed doing these changes? - Amruth ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37934/#review98361 ----------------------------------------------------------- On Sept. 9, 2015, 7:23 p.m., Amruth Sampath wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37934/ > ----------------------------------------------------------- > > (Updated Sept. 9, 2015, 7:23 p.m.) > > > Review request for lens, Amareshwari Sriramadasu, Pranav Agarwal, Rajat > Khandelwal, and sharad agarwal. > > > Repository: lens > > > Description > ------- > > Refer to the JIRA description - https://issues.apache.org/jira/browse/LENS-742 > (Note : Sharing and CLI are not a part of this patch) > > > Diffs > ----- > > lens-api/src/main/java/org/apache/lens/api/error/LensCommonErrorCode.java > 754e6e1 > lens-api/src/main/java/org/apache/lens/api/query/save/ListResponse.java > PRE-CREATION > lens-api/src/main/java/org/apache/lens/api/query/save/Parameter.java > PRE-CREATION > > lens-api/src/main/java/org/apache/lens/api/query/save/ParameterCollectionType.java > PRE-CREATION > > lens-api/src/main/java/org/apache/lens/api/query/save/ParameterDataType.java > PRE-CREATION > > lens-api/src/main/java/org/apache/lens/api/query/save/ParameterParserResponse.java > PRE-CREATION > > lens-api/src/main/java/org/apache/lens/api/query/save/ResourceModifiedResponse.java > PRE-CREATION > lens-api/src/main/java/org/apache/lens/api/query/save/SavedQuery.java > PRE-CREATION > lens-api/src/main/resources/lens-errors.conf 5428041 > > lens-server-api/src/main/java/org/apache/lens/server/api/LensConfConstants.java > fb11f93 > > lens-server-api/src/main/java/org/apache/lens/server/api/query/save/SavedQueryHelper.java > PRE-CREATION > > lens-server-api/src/main/java/org/apache/lens/server/api/query/save/SavedQueryService.java > PRE-CREATION > > lens-server-api/src/main/java/org/apache/lens/server/api/query/save/exception/MissingParameterException.java > PRE-CREATION > > lens-server-api/src/main/java/org/apache/lens/server/api/query/save/exception/ParameterCollectionException.java > PRE-CREATION > > lens-server-api/src/main/java/org/apache/lens/server/api/query/save/exception/ParameterValueException.java > PRE-CREATION > > lens-server-api/src/main/java/org/apache/lens/server/api/query/save/exception/PrivilegeException.java > PRE-CREATION > > lens-server-api/src/main/java/org/apache/lens/server/api/query/save/exception/SavedQueryNotFound.java > PRE-CREATION > > lens-server-api/src/main/java/org/apache/lens/server/api/query/save/exception/ValueEncodeException.java > PRE-CREATION > > lens-server-api/src/main/java/org/apache/lens/server/api/query/save/param/ParameterCollectionTypeEncoder.java > PRE-CREATION > > lens-server-api/src/main/java/org/apache/lens/server/api/query/save/param/ParameterDataTypeEncoder.java > PRE-CREATION > > lens-server-api/src/main/java/org/apache/lens/server/api/query/save/param/ParameterParser.java > PRE-CREATION > > lens-server-api/src/main/java/org/apache/lens/server/api/query/save/param/ParameterResolver.java > PRE-CREATION > > lens-server-api/src/test/java/org/apache/lens/server/api/query/save/TestParameterParser.java > PRE-CREATION > > lens-server-api/src/test/java/org/apache/lens/server/api/query/save/TestParameterResolution.java > PRE-CREATION > lens-server/enunciate.xml 94b5199 > > lens-server/src/main/java/org/apache/lens/server/query/save/SavedQueryApp.java > PRE-CREATION > > lens-server/src/main/java/org/apache/lens/server/query/save/SavedQueryDao.java > PRE-CREATION > > lens-server/src/main/java/org/apache/lens/server/query/save/SavedQueryResource.java > PRE-CREATION > > lens-server/src/main/java/org/apache/lens/server/query/save/SavedQueryServiceImpl.java > PRE-CREATION > lens-server/src/main/java/org/apache/lens/server/util/UtilityMethods.java > 5d77eb7 > lens-server/src/main/resources/lensserver-default.xml 5d33eda > > lens-server/src/test/java/org/apache/lens/server/query/save/TestSavedQueryService.java > PRE-CREATION > lens-server/src/test/resources/lens-site.xml 4cf94d5 > src/site/apt/admin/config.apt b163a3a > src/site/apt/user/index.apt 6a86b1b > > Diff: https://reviews.apache.org/r/37934/diff/ > > > Testing > ------- > > Have added unit test cases for parsing, resolution of parameter and service > testing. > > > Thanks, > > Amruth Sampath > >
