> On April 6, 2015, 9:30 p.m., Himanshu Gahlaut wrote: > > lens-cli/src/main/java/org/apache/lens/cli/commands/LensCubeCommands.java, > > line 154 > > <https://reviews.apache.org/r/32871/diff/1/?file=916304#file916304line154> > > > > From spring shell documentation, using multiple cli options seems to be > > a convention, instead of splitting a value in code. > > > > > > http://docs.spring.io/spring-shell/docs/current/reference/htmlsingle/#d4e220
http://issues.apache.org/jira/browse/LENS-256 and https://reviews.apache.org/r/30870/ Untill that's merged, let's keep the current convention. Because there are some issues over there. Let's discuss these issues there. > On April 6, 2015, 9:30 p.m., Himanshu Gahlaut wrote: > > lens-cli/src/main/java/org/apache/lens/cli/commands/LensCubeCommands.java, > > line 160 > > <https://reviews.apache.org/r/32871/diff/1/?file=916304#file916304line160> > > > > Can we have a @NonNull constraint on Date object in DateTime one > > argument constructor ? It will make sure that dateTime.getDate() can never > > be null and we will not have to handle it here. > > > > Also accesslevel of NoArgsConstructor of DateTime which is only > > required for JAXB can be reduced to AccessLevel.PACKAGE. I don't think making Date object non null makes sense here. Then we might as well use a Date object directly instead of DateTime. And now that I think of it, we can just use `Optional<Date>` instead of `DateTime`. > On April 6, 2015, 9:30 p.m., Himanshu Gahlaut wrote: > > lens-cli/src/test/java/org/apache/lens/cli/TestLensCubeCommands.java, line > > 57 > > <https://reviews.apache.org/r/32871/diff/1/?file=916305#file916305line57> > > > > Please consider adding separate modular test case for new command. I wanted to have minimal diff. Cleanup of test cases can be separate task. We have several issues: modularity, relative paths, coverage etc. > On April 6, 2015, 9:30 p.m., Himanshu Gahlaut wrote: > > lens-cli/src/test/java/org/apache/lens/cli/TestLensFactCommands.java, line > > 56 > > <https://reviews.apache.org/r/32871/diff/1/?file=916306#file916306line56> > > > > Please help in understanding, how createSampleCube and dropSampleCube > > belong to testFactCommands method ? inside `TestCubeCommands`, no facts are created so you can only test the null value and exception case. Only when facts are there and some partitions are registered, can we test for a value of latest. `CubeMetastoreClient` ***requires the cubes of all facts to be present*** for computing latest date. Now code-wise I don't see any such restriction is required, but our data model says that facts belong to a cube so requiring cube presence isn't something I could object to. Let me know if you have some arguments against the requirement. - Rajat ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32871/#review78924 ----------------------------------------------------------- On April 6, 2015, 12:54 p.m., Rajat Khandelwal wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/32871/ > ----------------------------------------------------------- > > (Updated April 6, 2015, 12:54 p.m.) > > > Review request for lens. > > > Bugs: LENS-474 > https://issues.apache.org/jira/browse/LENS-474 > > > Repository: lens > > > Description > ------- > > added method > > > Diffs > ----- > > lens-cli/src/main/java/org/apache/lens/cli/commands/BaseLensCommand.java > cdfdbe84c75640f438c887166ead8f64bbdda3f2 > lens-cli/src/main/java/org/apache/lens/cli/commands/LensCubeCommands.java > 9d835da2c30e096ca77558e075930f0d912e8c17 > lens-cli/src/test/java/org/apache/lens/cli/TestLensCubeCommands.java > 41f2c9336c3ec25ecfbe8ba289eab462d69844d5 > lens-cli/src/test/java/org/apache/lens/cli/TestLensFactCommands.java > f22515508cd4ceaf7b67ba4cb40ce0df8bf2ac6a > lens-client/src/main/java/org/apache/lens/client/LensClient.java > 8f1b7e18c1cc1a44bfd3c34dc84d9aad86902c9f > lens-client/src/main/java/org/apache/lens/client/LensMetadataClient.java > 3462dc3d6af807ef248e9ee0fbdff27cd0137b2c > > Diff: https://reviews.apache.org/r/32871/diff/ > > > Testing > ------- > > [INFO] > ------------------------------------------------------------------------ > [INFO] Reactor Summary: > [INFO] > [INFO] Lens Checkstyle Rules ............................. SUCCESS [2.150s] > [INFO] Lens .............................................. SUCCESS [2.268s] > [INFO] Lens API .......................................... SUCCESS [5.534s] > [INFO] Lens API for server and extensions ................ SUCCESS [6.637s] > [INFO] Lens Cube ......................................... SUCCESS [2:16.913s] > [INFO] Lens DB storage ................................... SUCCESS [10.299s] > [INFO] Lens Query Library ................................ SUCCESS [5.522s] > [INFO] Lens Hive Driver .................................. SUCCESS [2:33.670s] > [INFO] Lens Driver for JDBC .............................. SUCCESS [18.866s] > [INFO] Lens Server ....................................... SUCCESS [4:27.024s] > [INFO] Lens client ....................................... SUCCESS [20.632s] > [INFO] Lens CLI .......................................... SUCCESS [2:42.824s] > [INFO] Lens Examples ..................................... SUCCESS [0.848s] > [INFO] Lens Distribution ................................. SUCCESS [10.130s] > [INFO] Lens ML Lib ....................................... SUCCESS [1:02.403s] > [INFO] Lens ML Ext Distribution .......................... SUCCESS [2.515s] > [INFO] Lens Regression ................................... SUCCESS [0.483s] > [INFO] > ------------------------------------------------------------------------ > [INFO] BUILD SUCCESS > [INFO] > ------------------------------------------------------------------------ > [INFO] Total time: 14:29.632s > [INFO] Finished at: Mon Apr 06 07:11:15 UTC 2015 > [INFO] Final Memory: 108M/944M > [INFO] > ------------------------------------------------------------------------ > > > Thanks, > > Rajat Khandelwal > >
