> On Aug. 12, 2016, 2:37 p.m., Robert Nettleton wrote: > > Ship It!
This patch looks fine to me. One thing I noticed, and is not a problem with this current patch, is that the Configuration class is large, and quite unruly at this point with respect to maintenance. The patch proposed here is a great step forward, but I would recommend we possibly consider moving to a metadata file to define the configuration properties in ambari.properties. Having all these properties defined in Configuration.java really does make this code harder to maintain over time. That being said, I think having the ability to generate docs for these properties is a great improvement, I just wonder if over time we might want to consider moving to a separate config file, rather than include all this in code. Thanks. - Robert ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51004/#review145622 ----------------------------------------------------------- On Aug. 12, 2016, 1:28 p.m., Jonathan Hurley wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51004/ > ----------------------------------------------------------- > > (Updated Aug. 12, 2016, 1:28 p.m.) > > > Review request for Ambari, Alejandro Fernandez, Nate Cole, Robert Levas, > Robert Nettleton, and Sumit Mohanty. > > > Bugs: AMBARI-18126 > https://issues.apache.org/jira/browse/AMBARI-18126 > > > Repository: ambari > > > Description > ------- > > As part of the work for AMBARI-18089 > (https://issues.apache.org/jira/browse/AMBARI-18089), all of the potential > properties from {{ambari.properties}} need a way to be organized and queried > in order to generate documentation. > > This Jira proposes to change our disorganized String/String (key/default) > structure found in {{Configuration}}, and instead, have a strongly-coupled > relationship between key/default/type. > > Additionally, expose a way to provide extra information in the markdown > (similar to JavaDoc, but geared more for users and not developers). > > Some information on this patch: > - The goal was to get the refactor done; I'm not worried about the empty > JavaDoc/Markdown comments. I have > https://issues.apache.org/jira/browse/AMBARI-18089 tracking that work. > - This patch does generate markdown showing the properties and their defaults > - I plan on adding unit tests which ensure that every ConfigurationProperty > is annotated with a Markdown annotation. We need to be able to enforce good > documentation on configurations moving forward. > - Most of the changes are in Configuration.java; the other classes just > changed as a result. Most of them were tests anyway. > > > Diffs > ----- > > ambari-server/src/main/java/org/apache/ambari/annotations/Markdown.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/annotations/TransactionalLock.java > cd961ba > > ambari-server/src/main/java/org/apache/ambari/server/api/services/ComponentService.java > e7e0029 > > ambari-server/src/main/java/org/apache/ambari/server/api/services/HostComponentService.java > 72351c3 > > ambari-server/src/main/java/org/apache/ambari/server/checks/DatabaseConsistencyCheckHelper.java > 36a2d99 > > ambari-server/src/main/java/org/apache/ambari/server/configuration/ComponentSSLConfiguration.java > cb9651e > > ambari-server/src/main/java/org/apache/ambari/server/configuration/Configuration.java > fae9378 > > ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariServer.java > a6d8d6a > > ambari-server/src/main/java/org/apache/ambari/server/controller/ControllerModule.java > 2bd7eff > > ambari-server/src/main/java/org/apache/ambari/server/controller/KerberosHelperImpl.java > e8b0f15 > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClientConfigResourceProvider.java > c390c86 > > ambari-server/src/main/java/org/apache/ambari/server/controller/utilities/DatabaseChecker.java > d35fc1a > > ambari-server/src/main/java/org/apache/ambari/server/resources/ResourceManager.java > 9f4e708 > > ambari-server/src/main/java/org/apache/ambari/server/security/AbstractSecurityHeaderFilter.java > 10aa6ea > > ambari-server/src/main/java/org/apache/ambari/server/security/CertificateManager.java > 9f70dc0 > > ambari-server/src/main/java/org/apache/ambari/server/security/encryption/MasterKeyServiceImpl.java > 6c52cf4 > > ambari-server/src/main/java/org/apache/ambari/server/security/unsecured/rest/ConnectionInfo.java > 6e8b2aa > > ambari-server/src/main/java/org/apache/ambari/server/state/services/RetryUpgradeActionService.java > 7d81cc4 > > ambari-server/src/main/java/org/apache/ambari/server/state/stack/OsFamily.java > 599696a > > ambari-server/src/main/java/org/apache/ambari/server/view/ViewURLStreamProvider.java > 0e50f04 > > ambari-server/src/test/java/org/apache/ambari/annotations/TransactionalLockTest.java > 2c4b445 > > ambari-server/src/test/java/org/apache/ambari/server/actionmanager/TestActionScheduler.java > 9f12a94 > > ambari-server/src/test/java/org/apache/ambari/server/agent/TestHeartbeatHandler.java > 525595a > > ambari-server/src/test/java/org/apache/ambari/server/api/rest/KdcServerConnectionVerificationTest.java > 18a4c0a > > ambari-server/src/test/java/org/apache/ambari/server/api/services/AmbariMetaInfoTest.java > 538fa48 > > ambari-server/src/test/java/org/apache/ambari/server/api/services/KerberosServiceMetaInfoTest.java > befd594 > > ambari-server/src/test/java/org/apache/ambari/server/bootstrap/BootStrapTest.java > e4a385f > > ambari-server/src/test/java/org/apache/ambari/server/checks/UpgradeCheckOrderTest.java > 687d263 > > ambari-server/src/test/java/org/apache/ambari/server/cleanup/CleanupServiceFunctionalTest.java > 33018bc > > ambari-server/src/test/java/org/apache/ambari/server/configuration/ComponentSSLConfigurationTest.java > 92a7ef5 > > ambari-server/src/test/java/org/apache/ambari/server/configuration/ConfigurationTest.java > e9aebc0 > > ambari-server/src/test/java/org/apache/ambari/server/controller/AmbariManagementControllerTest.java > 420c078 > > ambari-server/src/test/java/org/apache/ambari/server/controller/BackgroundCustomCommandExecutionTest.java > 0f5224d > > ambari-server/src/test/java/org/apache/ambari/server/controller/internal/AlertResourceProviderTest.java > 93dff82 > > ambari-server/src/test/java/org/apache/ambari/server/controller/internal/ClientConfigResourceProviderTest.java > f0bddf8 > > ambari-server/src/test/java/org/apache/ambari/server/controller/internal/ClusterStackVersionResourceProviderTest.java > 24d4cc7 > > ambari-server/src/test/java/org/apache/ambari/server/controller/internal/CredentialResourceProviderTest.java > df4d8ed > > ambari-server/src/test/java/org/apache/ambari/server/controller/utilities/DatabaseCheckerTest.java > f148da1 > > ambari-server/src/test/java/org/apache/ambari/server/notifications/DispatchFactoryTest.java > 4f06ee2 > > ambari-server/src/test/java/org/apache/ambari/server/orm/InMemoryDefaultTestModule.java > 1ca486e > > ambari-server/src/test/java/org/apache/ambari/server/orm/JdbcPropertyTest.java > 1fac11b > > ambari-server/src/test/java/org/apache/ambari/server/resources/TestResources.java > 509a2ec > > ambari-server/src/test/java/org/apache/ambari/server/scheduler/ExecutionScheduleManagerTest.java > 860e647 > > ambari-server/src/test/java/org/apache/ambari/server/scheduler/ExecutionSchedulerTest.java > 8346c65 > > ambari-server/src/test/java/org/apache/ambari/server/security/AbstractSecurityHeaderFilterTest.java > 48231b7 > > ambari-server/src/test/java/org/apache/ambari/server/security/AmbariServerSecurityHeaderFilterTest.java > e6a8eff > > ambari-server/src/test/java/org/apache/ambari/server/security/AmbariViewsSecurityHeaderFilterTest.java > a2882ae > > ambari-server/src/test/java/org/apache/ambari/server/security/CertGenerationTest.java > 9fa8c58 > > ambari-server/src/test/java/org/apache/ambari/server/security/CertificateManagerTest.java > f28f234 > > ambari-server/src/test/java/org/apache/ambari/server/security/SslExecutionTest.java > 95a3f0a > > ambari-server/src/test/java/org/apache/ambari/server/security/authorization/AmbariLdapAuthenticationProviderForDuplicateUserTest.java > 02e4021 > > ambari-server/src/test/java/org/apache/ambari/server/security/authorization/AmbariLdapAuthenticationProviderTest.java > c011fc8 > > ambari-server/src/test/java/org/apache/ambari/server/security/authorization/AmbariLdapBindAuthenticatorTest.java > 27e62e2 > > ambari-server/src/test/java/org/apache/ambari/server/security/authorization/AuthorizationTestModule.java > ce4d25b > > ambari-server/src/test/java/org/apache/ambari/server/security/authorization/AuthorizationTestModuleForLdapDNWithSpace.java > 0e36930 > > ambari-server/src/test/java/org/apache/ambari/server/security/authorization/TestUsers.java > aa70be2 > > ambari-server/src/test/java/org/apache/ambari/server/security/encryption/CredentialProviderTest.java > 884cffa > > ambari-server/src/test/java/org/apache/ambari/server/security/encryption/CredentialStoreServiceImplTest.java > daea963 > > ambari-server/src/test/java/org/apache/ambari/server/security/encryption/MasterKeyServiceTest.java > b597e0a > > ambari-server/src/test/java/org/apache/ambari/server/security/ldap/LdapPerformanceTest.java > a816da6 > > ambari-server/src/test/java/org/apache/ambari/server/state/stack/ConfigUpgradeValidityTest.java > 0c45347 > > ambari-server/src/test/java/org/apache/ambari/server/upgrade/UpgradeTest.java > a1fd32d > > ambari-server/src/test/java/org/apache/ambari/server/view/ViewURLStreamProviderTest.java > 89df7fb > > Diff: https://reviews.apache.org/r/51004/diff/ > > > Testing > ------- > > Known failing test on b.a.o > > Tests in error: > AmbariManagementControllerTest.testScheduleSmokeTest:9757 ยป Rollback > Exception... > > Tests run: 4612, Failures: 0, Errors: 1, Skipped: 34 > > [INFO] > ------------------------------------------------------------------------ > [INFO] BUILD FAILURE > [INFO] > ------------------------------------------------------------------------ > [INFO] Total time: 01:04 h > [INFO] Finished at: 2016-08-11T18:11:29-04:00 > [INFO] Final Memory: 35M/601M > [INFO] > ------------------------------------------------------------------------ > > > Thanks, > > Jonathan Hurley > >
