----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46562/#review130498 -----------------------------------------------------------
Did a quick pass through - could we add JavaDoc for methods with don't have it - I saw a lot of them. I'll take another pass through and look at some of the view specific code ambari-server/src/main/java/org/apache/ambari/server/api/resources/ResourceInstanceFactoryImpl.java (lines 225 - 227) <https://reviews.apache.org/r/46562/#comment194234> It's not really an instance of a view or a cluster - it's just configuration information, right? Should probably reflect that in the naming; something like ViewClusterConfiguration ambari-server/src/main/java/org/apache/ambari/server/api/resources/ResourceInstanceFactoryImpl.java (lines 229 - 231) <https://reviews.apache.org/r/46562/#comment194235> This really isn't a service; it's more like the meta information which describes what a view needs. Could be something like ViewConfigurationTemplate or something similar. ambari-server/src/main/java/org/apache/ambari/server/api/services/ViewClusterInstanceService.java (line 35) <https://reviews.apache.org/r/46562/#comment194239> Consider revising / renaming. ambari-server/src/main/java/org/apache/ambari/server/api/services/ViewClusterInstanceService.java (lines 47 - 51) <https://reviews.apache.org/r/46562/#comment194236> GETs don't need a body. ambari-server/src/main/java/org/apache/ambari/server/api/services/ViewServiceService.java (line 38) <https://reviews.apache.org/r/46562/#comment194238> Consider revising / renaming. ambari-server/src/main/java/org/apache/ambari/server/api/services/ViewServiceService.java (lines 50 - 54) <https://reviews.apache.org/r/46562/#comment194237> Gets don't need a body. ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ViewClusterInstanceResourceProvider.java (lines 138 - 139) <https://reviews.apache.org/r/46562/#comment194244> Proper Javadoc ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ViewClusterInstanceResourceProvider.java (line 141) <https://reviews.apache.org/r/46562/#comment194240> Transactional should not be defined at this level. ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ViewClusterInstanceResourceProvider.java (line 165) <https://reviews.apache.org/r/46562/#comment194241> Transactional should not be defined at this level. ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ViewClusterInstanceResourceProvider.java (line 182) <https://reviews.apache.org/r/46562/#comment194243> JavaDoc ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ViewClusterInstanceResourceProvider.java (line 198) <https://reviews.apache.org/r/46562/#comment194242> JavaDoc. ambari-server/src/main/java/org/apache/ambari/server/orm/dao/ViewClusterConfigurationDao.java (lines 36 - 40) <https://reviews.apache.org/r/46562/#comment194247> Private? ambari-server/src/main/java/org/apache/ambari/server/orm/dao/ViewClusterConfigurationDao.java (lines 67 - 69) <https://reviews.apache.org/r/46562/#comment194248> Use a NamedQuery ambari-server/src/main/java/org/apache/ambari/server/orm/dao/ViewServiceDAO.java (lines 41 - 42) <https://reviews.apache.org/r/46562/#comment194249> Private? ambari-server/src/main/java/org/apache/ambari/server/orm/entities/ViewClusterConfigurationPropertyEntityPK.java (line 27) <https://reviews.apache.org/r/46562/#comment194250> We don't use compound PKs - instead use a surrogate PK and define these parameters as a UNIQUE constraint. ambari-server/src/main/java/org/apache/ambari/server/orm/entities/ViewClusterServiceEntityPK.java (line 27) <https://reviews.apache.org/r/46562/#comment194246> We don't use compound PKs - instead use a surrogate PK and define these parameters as a UNIQUE constraint. ambari-server/src/main/java/org/apache/ambari/server/orm/entities/ViewInstanceEntity.java (lines 86 - 88) <https://reviews.apache.org/r/46562/#comment194251> Enum? ambari-server/src/main/java/org/apache/ambari/server/orm/entities/ViewInstanceEntity.java (lines 124 - 125) <https://reviews.apache.org/r/46562/#comment194245> This should be an enumeration. ambari-server/src/main/java/org/apache/ambari/server/orm/entities/ViewServiceParameterEntityPK.java (line 27) <https://reviews.apache.org/r/46562/#comment194252> We don't use compound PKs - instead use a surrogate PK and define these parameters as a UNIQUE constraint. ambari-server/src/main/resources/Ambari-DDL-Derby-CREATE.sql (lines 861 - 900) <https://reviews.apache.org/r/46562/#comment194254> In general we use surrogate PKs and try not to use the business logic as constraints. Also, we can create the PK and FK relationships inline with a name for the constraint. This goes for all SQL files. - Jonathan Hurley On April 22, 2016, 3:32 p.m., Gaurav Nagar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/46562/ > ----------------------------------------------------------- > > (Updated April 22, 2016, 3:32 p.m.) > > > Review request for Ambari, DIPAYAN BHOWMICK, Nitiraj Rathore, Pallav > Kulshreshtha, Rohit Choudhary, and Ashwin Rajeev. > > > Bugs: AMBARI-16037 > https://issues.apache.org/jira/browse/AMBARI-16037 > > > Repository: ambari > > > Description > ------- > > Added view service definition which contains the service dependecy configs > and related configuration required for services. Each view can define > dependency on these services. > Added Remote cluster definition where user will be able to configure it once > and view instances can be linked to any one of them. Thus, the configurations > will be created once and used multiple times. > > > Diffs > ----- > > ambari-admin/src/main/resources/ui/admin-web/app/index.html fa911a6 > > ambari-admin/src/main/resources/ui/admin-web/app/scripts/controllers/ambariViews/CreateViewInstanceCtrl.js > 962b795 > > ambari-admin/src/main/resources/ui/admin-web/app/scripts/controllers/ambariViews/RemoteClusterInstanceCtrl.js > PRE-CREATION > > ambari-admin/src/main/resources/ui/admin-web/app/scripts/controllers/ambariViews/RemoteClusterListCtrl.js > PRE-CREATION > > ambari-admin/src/main/resources/ui/admin-web/app/scripts/controllers/ambariViews/ViewsEditCtrl.js > d46a30f > ambari-admin/src/main/resources/ui/admin-web/app/scripts/i18n.config.js > e95ebdb > ambari-admin/src/main/resources/ui/admin-web/app/scripts/routes.js 4fc4ea6 > > ambari-admin/src/main/resources/ui/admin-web/app/scripts/services/RemoteCluster.js > PRE-CREATION > ambari-admin/src/main/resources/ui/admin-web/app/scripts/services/View.js > cbe11e4 > > ambari-admin/src/main/resources/ui/admin-web/app/views/ambariViews/create.html > 20ccadb > > ambari-admin/src/main/resources/ui/admin-web/app/views/ambariViews/edit.html > b41abc8 > > ambari-admin/src/main/resources/ui/admin-web/app/views/ambariViews/listClusters.html > PRE-CREATION > > ambari-admin/src/main/resources/ui/admin-web/app/views/ambariViews/viewClusters.html > PRE-CREATION > ambari-admin/src/main/resources/ui/admin-web/app/views/leftNavbar.html > 9bc54ff > > ambari-server/src/main/java/org/apache/ambari/server/api/resources/ResourceInstanceFactoryImpl.java > eed2703 > > ambari-server/src/main/java/org/apache/ambari/server/api/resources/ViewClusterInstanceResourceDefinition.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/api/resources/ViewServiceResourceDefinition.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/api/services/ViewClusterInstanceService.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/api/services/ViewServiceService.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariServer.java > dc53172 > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/DefaultProviderModule.java > c7dc117 > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ViewClusterInstanceResourceProvider.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ViewInstanceResourceProvider.java > 6523962 > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ViewServiceResourceProvider.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ViewVersionResourceProvider.java > 1bf750b > > ambari-server/src/main/java/org/apache/ambari/server/controller/spi/Resource.java > 5a8476d > > ambari-server/src/main/java/org/apache/ambari/server/notifications/dispatchers/AlertScriptDispatcher.java > 907588d > > ambari-server/src/main/java/org/apache/ambari/server/orm/dao/ViewClusterConfigurationDao.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/orm/dao/ViewServiceDAO.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/orm/entities/ViewClusterConfigurationEntity.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/orm/entities/ViewClusterConfigurationPropertyEntity.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/orm/entities/ViewClusterConfigurationPropertyEntityPK.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/orm/entities/ViewClusterServiceEntity.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/orm/entities/ViewClusterServiceEntityPK.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/orm/entities/ViewEntity.java > 29dc2a7 > > ambari-server/src/main/java/org/apache/ambari/server/orm/entities/ViewInstanceEntity.java > 2555f93 > > ambari-server/src/main/java/org/apache/ambari/server/orm/entities/ViewParameterEntity.java > 5419d58 > > ambari-server/src/main/java/org/apache/ambari/server/orm/entities/ViewServiceEntity.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/orm/entities/ViewServiceParameterEntity.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/orm/entities/ViewServiceParameterEntityPK.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/upgrade/UpgradeCatalog240.java > 3583dd1 > > ambari-server/src/main/java/org/apache/ambari/server/view/ViewArchiveUtility.java > d1ead32 > > ambari-server/src/main/java/org/apache/ambari/server/view/ViewContextImpl.java > ba7f446 > ambari-server/src/main/java/org/apache/ambari/server/view/ViewRegistry.java > 7379f37 > > ambari-server/src/main/java/org/apache/ambari/server/view/configuration/ParameterConfig.java > 8e686eb > > ambari-server/src/main/java/org/apache/ambari/server/view/configuration/ServiceConfig.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/view/configuration/ServiceParameterConfig.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/view/configuration/ViewConfig.java > 728f620 > ambari-server/src/main/resources/Ambari-DDL-Derby-CREATE.sql 93576f7 > ambari-server/src/main/resources/Ambari-DDL-MySQL-CREATE.sql b0264f2 > ambari-server/src/main/resources/Ambari-DDL-Oracle-CREATE.sql 56a6616 > ambari-server/src/main/resources/Ambari-DDL-Postgres-CREATE.sql f18cdec > ambari-server/src/main/resources/Ambari-DDL-Postgres-EMBEDDED-CREATE.sql > 4584d5e > ambari-server/src/main/resources/Ambari-DDL-SQLAnywhere-CREATE.sql d2737d7 > ambari-server/src/main/resources/Ambari-DDL-SQLServer-CREATE.sql c85ae46 > ambari-server/src/main/resources/META-INF/persistence.xml 3eff96f > > ambari-server/src/test/java/org/apache/ambari/server/api/resources/ViewClusterInstanceResourceDefinitionTest.java > PRE-CREATION > > ambari-server/src/test/java/org/apache/ambari/server/controller/internal/ViewClusterInstanceResourceProviderTest.java > PRE-CREATION > > ambari-server/src/test/java/org/apache/ambari/server/controller/internal/ViewInstanceResourceProviderTest.java > aedac18 > > ambari-server/src/test/java/org/apache/ambari/server/controller/internal/ViewServiceResourceProviderTest.java > PRE-CREATION > > ambari-server/src/test/java/org/apache/ambari/server/orm/entities/ViewClusterConfigurationEntityTest.java > PRE-CREATION > > ambari-server/src/test/java/org/apache/ambari/server/orm/entities/ViewEntityTest.java > 1022e7c > > ambari-server/src/test/java/org/apache/ambari/server/orm/entities/ViewInstanceEntityTest.java > c8c15da > > ambari-server/src/test/java/org/apache/ambari/server/orm/entities/ViewServiceEntityTest.java > PRE-CREATION > > ambari-server/src/test/java/org/apache/ambari/server/orm/entities/ViewServiceParameterEntityTest.java > PRE-CREATION > > ambari-server/src/test/java/org/apache/ambari/server/upgrade/UpgradeCatalog240Test.java > 6439401 > > ambari-server/src/test/java/org/apache/ambari/server/view/ViewArchiveUtilityTest.java > aff29f4 > > ambari-server/src/test/java/org/apache/ambari/server/view/ViewContextImplTest.java > 7e2d9b5 > > ambari-server/src/test/java/org/apache/ambari/server/view/ViewRegistryTest.java > 5b24b19 > > ambari-server/src/test/java/org/apache/ambari/server/view/configuration/ParameterConfigTest.java > ce599b6 > > ambari-server/src/test/java/org/apache/ambari/server/view/configuration/ServiceConfigTest.java > PRE-CREATION > > ambari-server/src/test/java/org/apache/ambari/server/view/configuration/ViewConfigTest.java > 14e5d3c > > ambari-views/examples/auto-cluster-view/src/main/resources/auto-cluster-view-service.xml > PRE-CREATION > ambari-views/examples/auto-cluster-view/src/main/resources/view.xml 94ad0ed > > ambari-views/examples/cluster-view/src/main/resources/cluster-view-service.xml > PRE-CREATION > ambari-views/examples/cluster-view/src/main/resources/view.xml d53e5a5 > > ambari-views/src/main/java/org/apache/ambari/view/ViewInstanceDefinition.java > 62f9657 > ambari-views/src/main/java/org/apache/ambari/view/validation/Validator.java > ee029a8 > ambari-views/src/main/resources/view-service.xsd PRE-CREATION > ambari-views/src/main/resources/view.xsd c3ad711 > > contrib/views/capacity-scheduler/src/main/java/org/apache/ambari/view/capacityscheduler/PropertyValidator.java > a4976c7 > contrib/views/capacity-scheduler/src/main/resources/view.xml 6dc3ffa > contrib/views/files/pom.xml 383d90d > > contrib/views/files/src/main/java/org/apache/ambari/view/filebrowser/PropertyValidator.java > 2ad779c > contrib/views/files/src/main/resources/hdfs-service.xml PRE-CREATION > contrib/views/files/src/main/resources/hive-service.xml PRE-CREATION > contrib/views/files/src/main/resources/remotecluster-service.xml > PRE-CREATION > contrib/views/files/src/main/resources/view.xml ad5202c > contrib/views/files/src/main/resources/webhcat-service.xml PRE-CREATION > contrib/views/files/src/main/resources/yarn-service.xml PRE-CREATION > > contrib/views/hive/src/main/java/org/apache/ambari/view/hive/PropertyValidator.java > ae73bc0 > contrib/views/hive/src/main/resources/view.xml 8f8a470 > > contrib/views/pig/src/main/java/org/apache/ambari/view/pig/PropertyValidator.java > d3c9866 > contrib/views/pig/src/main/resources/view.xml 9df91f8 > contrib/views/slider/src/main/resources/view.xml f4f6e9e > contrib/views/tez/readme.md fdb9459 > > contrib/views/tez/src/main/java/org/apache/ambari/view/tez/ViewController.java > 440ac65 > contrib/views/tez/src/main/resources/view.xml d8105f1 > > contrib/views/utils/src/main/java/org/apache/ambari/view/utils/ambari/Services.java > a8ef43f > > contrib/views/utils/src/test/java/org/apache/ambari/view/utils/ambari/ServicesTest.java > 455ca20 > contrib/views/zeppelin/pom.xml 3d0161c > contrib/views/zeppelin/src/main/resources/view.xml 3c5c5cf > contrib/views/zeppelin/src/main/resources/zeppelin-service.xml PRE-CREATION > > Diff: https://reviews.apache.org/r/46562/diff/ > > > Testing > ------- > > Manually tested on local vm. > > > Thanks, > > Gaurav Nagar > >
