> On Aug. 12, 2016, 10:37 a.m., Robert Nettleton wrote:
> > Ship It!
> 
> Robert Nettleton wrote:
>     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.

Thanks for the review, Bob! 

I agree that this file is massive and hard to maintain. I think that the goal 
of this Jira was to first and foremost document what we have. Nobody knows 
anything about our config options with most of them flying under the radar. 
With the new documentation added (and the ability to generate MD 
automagically), we should be able to translate this to the next iteration of 
how we handle configurations in Ambari.

For now, I allocated ~2 days to this effort. I think moving to a better, 
more-manageable configuration framework would be beyond the 2-day scope. So 
this is a good starting point. If you have any suggestions about what we might 
want to move toward feel free to share them here or open a Jira where we can 
discuss.


- Jonathan


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51004/#review145622
-----------------------------------------------------------


On Aug. 12, 2016, 9:28 a.m., Jonathan Hurley wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51004/
> -----------------------------------------------------------
> 
> (Updated Aug. 12, 2016, 9:28 a.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
> 
>

Reply via email to