----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51238/#review147153 -----------------------------------------------------------
I was under the impression that applying an m-pack would DIRECTLY update repoinfo.xml on the filesystem. Has the design changed since then? What is the purpose of carrying service version around with the repo? When updating m-packs, that just means that there's that much more work to do to get it right. How are you going to prevent an m-pack from overwriting an existing repo? (What if an m-pack names a repo "HDP-UTILS" or something - then they just blew over ours)? You need to fix the code formatting so it's consistent and a bit easier to read. In general, I don't like the idea of using service name to tie a repo to a service. Maybe name this "reference" or a different indicator of where this repo came from. ambari-server/src/main/java/org/apache/ambari/server/controller/RepositoryResponse.java (lines 168 - 183) <https://reviews.apache.org/r/51238/#comment214257> Formatting. ambari-server/src/main/java/org/apache/ambari/server/controller/internal/VersionDefinitionResourceProvider.java (line 562) <https://reviews.apache.org/r/51238/#comment214268> Weird that it's setOperatingSystems() but getting RepositoryInfos() ambari-server/src/main/java/org/apache/ambari/server/stack/RepoUtil.java (lines 20 - 21) <https://reviews.apache.org/r/51238/#comment214260> expand imports. java/javax go first, then org, then com ambari-server/src/main/java/org/apache/ambari/server/stack/RepoUtil.java (line 95) <https://reviews.apache.org/r/51238/#comment214258> Javadoc ambari-server/src/main/java/org/apache/ambari/server/stack/RepoUtil.java (line 110) <https://reviews.apache.org/r/51238/#comment214259> Javadoc ambari-server/src/main/java/org/apache/ambari/server/stack/UpdateActiveRepoVersionOnStartup.java (line 122) <https://reviews.apache.org/r/51238/#comment214262> Why is the XML getting updated? It makes it "unpure". Updating the repos that are built FROM it should be sufficient. ambari-server/src/main/java/org/apache/ambari/server/state/RepositoryInfo.java (lines 21 - 23) <https://reviews.apache.org/r/51238/#comment214263> Ordering ambari-server/src/main/java/org/apache/ambari/server/state/RepositoryInfo.java (lines 213 - 215) <https://reviews.apache.org/r/51238/#comment214264> formatting. - Nate Cole On Aug. 26, 2016, 5:25 a.m., Balázs Bence Sári wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51238/ > ----------------------------------------------------------- > > (Updated Aug. 26, 2016, 5:25 a.m.) > > > Review request for Ambari, Jayush Luniya, Nate Cole, Sumit Mohanty, and > Sebastian Toader. > > > Bugs: AMBARI-15538 > https://issues.apache.org/jira/browse/AMBARI-15538 > > > Repository: ambari > > > Description > ------- > > Implementation contains the following things: > - On startup, add-on service repos are loaded into the stack model > - On startup, add-on service repos are merged into the VDF's downloaded by > LatestRepoCallable > - On startup, if the is an existing cluster, it's repository version entity > is potentially updated with new add-on service repos. > - Repository definitions contain two new optional fields: service_name and > service_version. (null for stack repositories) > - Small changes on the Mictrosoft-R mpack (supoorts HDP-2.4 and 2.5) > - New unit tests > > > Diffs > ----- > > > ambari-server/src/main/java/org/apache/ambari/server/api/services/AmbariMetaInfo.java > 92d47df > > ambari-server/src/main/java/org/apache/ambari/server/checks/DatabaseConsistencyCheckHelper.java > 36a2d99 > > ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementControllerImpl.java > 63c99c6 > > ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariServer.java > 097f01c > > ambari-server/src/main/java/org/apache/ambari/server/controller/RepositoryResponse.java > 30bd0db > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/RepositoryResourceProvider.java > 3b5b0a7 > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/VersionDefinitionResourceProvider.java > 02fc2ec > > ambari-server/src/main/java/org/apache/ambari/server/orm/entities/RepositoryEntity.java > 49d53a9 > > ambari-server/src/main/java/org/apache/ambari/server/orm/entities/RepositoryVersionEntity.java > 25aa62b > ambari-server/src/main/java/org/apache/ambari/server/stack/RepoUtil.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/stack/StackDirectory.java > bfba021 > ambari-server/src/main/java/org/apache/ambari/server/stack/StackModule.java > 0606f2a > > ambari-server/src/main/java/org/apache/ambari/server/stack/StackServiceDirectory.java > 7bcd08b > > ambari-server/src/main/java/org/apache/ambari/server/stack/UpdateActiveRepoVersionOnStartup.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/state/RepositoryInfo.java > 29776ed > ambari-server/src/main/java/org/apache/ambari/server/state/StackInfo.java > 14ff9de > > ambari-server/src/main/java/org/apache/ambari/server/state/stack/LatestRepoCallable.java > 3c7c001 > > ambari-server/src/main/java/org/apache/ambari/server/state/stack/RepositoryXml.java > 4a0ae3b > > ambari-server/src/main/java/org/apache/ambari/server/state/stack/upgrade/RepositoryVersionHelper.java > 50d6028 > > ambari-server/src/main/java/org/apache/ambari/server/state/svccomphost/ServiceComponentHostImpl.java > 3b5ed28 > > ambari-server/src/main/java/org/apache/ambari/server/upgrade/UpgradeCatalog220.java > d806dde > ambari-server/src/main/resources/version_definition.xsd bd49028 > > ambari-server/src/test/java/org/apache/ambari/server/controller/internal/RepositoryResourceProviderTest.java > 3a7b19b > > ambari-server/src/test/java/org/apache/ambari/server/stack/RepoUtilTest.java > PRE-CREATION > > ambari-server/src/test/java/org/apache/ambari/server/stack/StackManagerCommonServicesTest.java > 1d73ff3 > > ambari-server/src/test/java/org/apache/ambari/server/stack/StackModuleTest.java > PRE-CREATION > > ambari-server/src/test/java/org/apache/ambari/server/stack/UpdateActiveRepoVersionOnStartupTest.java > PRE-CREATION > > ambari-server/src/test/java/org/apache/ambari/server/state/stack/upgrade/RepositoryVersionHelperTest.java > 8d82d09 > > ambari-server/src/test/resources/common-services/ADDON/1.0/configuration/addon-env.xml > PRE-CREATION > ambari-server/src/test/resources/common-services/ADDON/1.0/metainfo.xml > PRE-CREATION > > ambari-server/src/test/resources/org/apache/ambari/server/stack/UpdateActiveRepoVersionOnStartupTest_initialRepos.json > PRE-CREATION > > ambari-server/src/test/resources/org/apache/ambari/server/stack/UpdateActiveRepoVersionOnStartupTest_initialRepos.xml > PRE-CREATION > > ambari-server/src/test/resources/stacks_with_common_services/HDP/0.2/services/ADDON/metainfo.xml > PRE-CREATION > > ambari-server/src/test/resources/stacks_with_common_services/HDP/0.2/services/ADDON/repos/repoinfo.xml > PRE-CREATION > > contrib/management-packs/microsoft-r_mpack/src/main/resources/common-services/MICROSOFT_R/8.0.0/configuration/microsoft-r-env.xml > PRE-CREATION > > contrib/management-packs/microsoft-r_mpack/src/main/resources/common-services/MICROSOFT_R/8.0.0/package/scripts/microsoft_r.py > 61ea96b > > contrib/management-packs/microsoft-r_mpack/src/main/resources/custom-services/MICROSOFT_R/8.0.0/repos/repoinfo.xml > PRE-CREATION > contrib/management-packs/microsoft-r_mpack/src/main/resources/mpack.json > f90ccce > > Diff: https://reviews.apache.org/r/51238/diff/ > > > Testing > ------- > > - Manually tested > - Wrote new unit tests > - All unit tests passed except two which were failing in CI builds as well. > > > Thanks, > > Balázs Bence Sári > >
