----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47023/#review132081 -----------------------------------------------------------
ambari-server/pom.xml (lines 1371 - 1375) <https://reviews.apache.org/r/47023/#comment196141> Why do we need yet another library for this? Can't the 2 that we already have suffice? ambari-server/pom.xml (lines 1376 - 1380) <https://reviews.apache.org/r/47023/#comment196143> Collections 3.2.1 is used by Apache DS and is on our path already. Is Apache DS compatible with this version? ambari-server/src/main/java/org/apache/ambari/server/controller/internal/RemoteClusterResourceProvider.java (lines 167 - 173) <https://reviews.apache.org/r/47023/#comment196149> Why not just search directly by name? ambari-server/src/main/java/org/apache/ambari/server/controller/internal/RemoteClusterResourceProvider.java (line 187) <https://reviews.apache.org/r/47023/#comment196151> This seems display-specific. We should probably let the display handle how it wants to format the list. ambari-server/src/main/java/org/apache/ambari/server/controller/internal/RemoteClusterResourceProvider.java (line 221) <https://reviews.apache.org/r/47023/#comment196152> This should not be transactional - that can be handled in the DAO. ambari-server/src/main/java/org/apache/ambari/server/controller/internal/RemoteClusterResourceProvider.java (line 249) <https://reviews.apache.org/r/47023/#comment196153> This should not be transactional - should be handled in the DAO. ambari-server/src/main/java/org/apache/ambari/server/controller/internal/RemoteClusterResourceProvider.java (lines 258 - 260) <https://reviews.apache.org/r/47023/#comment196154> The ResourceProvider shouldn't have knowledge of how things are associated on the backend; it should simply update the resource. The update is what should do the rest of the work, like refreshing the registry ambari-server/src/main/java/org/apache/ambari/server/controller/internal/RemoteClusterResourceProvider.java (line 295) <https://reviews.apache.org/r/47023/#comment196156> StringUtils.isBlank() for password to cover spaces and empty strings. ambari-server/src/main/java/org/apache/ambari/server/controller/internal/RemoteClusterResourceProvider.java (lines 340 - 341) <https://reviews.apache.org/r/47023/#comment196157> The ResourceProvider shouldn't be responsible for both deletes. ambari-server/src/main/java/org/apache/ambari/server/controller/internal/RemoteClusterResourceProvider.java (line 347) <https://reviews.apache.org/r/47023/#comment196158> JavaDoc. ambari-server/src/main/java/org/apache/ambari/server/controller/internal/RemoteClusterResourceProvider.java (lines 360 - 365) <https://reviews.apache.org/r/47023/#comment196159> This is a get method but it appears to also be creating services. ambari-server/src/main/java/org/apache/ambari/server/orm/dao/RemoteClusterServiceDAO.java (line 72) <https://reviews.apache.org/r/47023/#comment196160> No need for Optional here. ambari-server/src/main/java/org/apache/ambari/server/orm/entities/RemoteAmbariClusterEntity.java (line 59) <https://reviews.apache.org/r/47023/#comment196163> Methods in this class need some JavaDoc. ambari-server/src/main/java/org/apache/ambari/server/orm/entities/RemoteAmbariClusterEntity.java (lines 83 - 86) <https://reviews.apache.org/r/47023/#comment196161> Is this really a M2M relationship? RemoteClusterServiceEntity are shared between clusters? ambari-server/src/main/java/org/apache/ambari/server/orm/entities/RemoteAmbariClusterServiceEntity.java (line 33) <https://reviews.apache.org/r/47023/#comment196164> JavaDoc the methods. ambari-server/src/main/java/org/apache/ambari/server/orm/entities/ViewEntity.java (line 733) <https://reviews.apache.org/r/47023/#comment196165> Use the cluster type enum here? ambari-server/src/main/java/org/apache/ambari/server/view/LocalAmbariClusterStreamProvider.java (line 41) <https://reviews.apache.org/r/47023/#comment196166> How is this different from a ViewAmbariStreamProvider aside from the fact it's scoped to /api/v1/clusters - seems like you could just use ViewAmbariStreamProvider ambari-server/src/main/java/org/apache/ambari/server/view/RemoteAmbariCluster.java (line 103) <https://reviews.apache.org/r/47023/#comment196169> Could this be null? ambari-server/src/main/java/org/apache/ambari/server/view/RemoteAmbariCluster.java (line 111) <https://reviews.apache.org/r/47023/#comment196167> JavaDoc. ambari-server/src/main/java/org/apache/ambari/server/view/RemoteAmbariCluster.java (lines 112 - 115) <https://reviews.apache.org/r/47023/#comment196170> Lots of faith here that nothing is null... ambari-server/src/main/java/org/apache/ambari/server/view/RemoteAmbariCluster.java (line 120) <https://reviews.apache.org/r/47023/#comment196168> JavaDoc. ambari-server/src/main/java/org/apache/ambari/server/view/RemoteAmbariClusterRegistry.java (line 46) <https://reviews.apache.org/r/47023/#comment196171> Should not use a synchronized method. You can use some finer grained locks along with a volatile clusterMap. ambari-server/src/main/java/org/apache/ambari/server/view/RemoteAmbariClusterStreamProvider.java (line 42) <https://reviews.apache.org/r/47023/#comment196172> Same question from above - how is this different from a ViewAmbariStreamProvider aside from the fact it's scoped to /api/v1/clusters? - Jonathan Hurley On May 5, 2016, 12:16 p.m., Gaurav Nagar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/47023/ > ----------------------------------------------------------- > > (Updated May 5, 2016, 12:16 p.m.) > > > Review request for Ambari, DIPAYAN BHOWMICK, Jonathan Hurley, Nitiraj > Rathore, Pallav Kulshreshtha, Rohit Choudhary, and Ashwin Rajeev. > > > Bugs: AMBARI-16274 > https://issues.apache.org/jira/browse/AMBARI-16274 > > > Repository: ambari > > > Description > ------- > > Added RemotAmbariClusterEntity, RemoteAmbariClusterDao to store remote > cluster credential. > Added RemoteClusterResourceProvider for accessing Remote Cluster through api. > Added RemoteAmbariCluster impementation for view.Cluster that can be accessed > through ViewContextImpl. > > > Diffs > ----- > > ambari-server/pom.xml 20d3fab > > ambari-server/src/main/java/org/apache/ambari/server/api/resources/RemoteClusterResourceDefinition.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/api/resources/ResourceInstanceFactoryImpl.java > 0b77511 > > ambari-server/src/main/java/org/apache/ambari/server/api/services/RemoteClustersService.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/DefaultProviderModule.java > 4e7a032 > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/RemoteClusterResourceProvider.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ViewInstanceResourceProvider.java > 605f68d > > ambari-server/src/main/java/org/apache/ambari/server/controller/spi/Resource.java > 386e657 > > ambari-server/src/main/java/org/apache/ambari/server/orm/dao/RemoteAmbariClusterDAO.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/orm/dao/RemoteClusterServiceDAO.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/orm/entities/RemoteAmbariClusterEntity.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/orm/entities/RemoteAmbariClusterServiceEntity.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/orm/entities/RemoteClusterServiceEntity.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 > 2d6e5ba > > ambari-server/src/main/java/org/apache/ambari/server/view/LocalAmbariClusterStreamProvider.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/view/RemoteAmbariCluster.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/view/RemoteAmbariClusterRegistry.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/view/RemoteAmbariClusterStreamProvider.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/view/RemoteAmbariConfigurationReadException.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/view/ViewAmbariStreamProvider.java > 1dacd92 > > 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 > d2d48a9 > > ambari-server/src/main/java/org/apache/ambari/server/view/configuration/ViewConfig.java > bb6a93c > ambari-server/src/main/resources/META-INF/persistence.xml ce563cb > > ambari-server/src/test/java/org/apache/ambari/server/controller/internal/RemoteClusterResourceProviderTest.java > PRE-CREATION > > ambari-server/src/test/java/org/apache/ambari/server/view/RemoteAmbariClusterTest.java > PRE-CREATION > > ambari-server/src/test/java/org/apache/ambari/server/view/configuration/ViewConfigTest.java > a41e137 > > contrib/views/utils/src/main/java/org/apache/ambari/view/utils/ambari/RemoteCluster.java > 41dc88d > > Diff: https://reviews.apache.org/r/47023/diff/ > > > Testing > ------- > > Manual Testing > > > Thanks, > > Gaurav Nagar > >
