brooklyn-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From aledsage <...@git.apache.org>
Subject [GitHub] brooklyn-server pull request #390: BROOKLYN-356: fix race in transformer by ...
Date Tue, 01 Nov 2016 17:38:36 GMT
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/brooklyn-server/pull/390#discussion_r85981351
  
    --- Diff: camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/dsl/methods/BrooklynDslCommon.java
---
    @@ -525,15 +583,20 @@ public DslExternal(String providerName, String key) {
             }
     
             @Override
    +        public final Maybe<Object> getImmediately() {
    +            ManagementContextInternal managementContext = DslExternal.managementContext();
    +            return Maybe.<Object>of(managementContext.getExternalConfigProviderRegistry().getConfig(providerName,
key));
    --- End diff --
    
    @neykov This is an interesting one! The `managementContext.getExternalConfigProviderRegistry().getConfig(...)`
is calling a different getConfig whose javadoc simply says:
    
    ```
        /**
         * Searches the named {@link ExternalConfigSupplier} for the config value associated
with the specified key.
         * Quietly returns <code>null</code> if no config exists for the specified
key.
         * Throws {@link IllegalArgumentException} if no {@link ExternalConfigSupplier} exists
for the passed name.
         */
    ```
    
    The existing implementation delegates to `ExternalConfigSupplier.get(key)`.
    
    There is no guidance on that whether the call could be blocking, or should return reasonably
promptly.
    
    I'd assume it would call out to things like Vault, and would return the value that currently
exists or null. Such calls will not be super-fast, but should not be blocked waiting for other
entities either.
    
    I therefore think this is good enough, to just call getConfig without wrapping it in another
task for timeout-purposes.
    
    If it's not good enough, then we should really change `DslExternal` back so that it doesn't
even try to support `getImmediately()`. Or we could try to change the interface of `ExternalConfigSupplierRegistry`
etc.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

Mime
View raw message