[RESTEASY-2525]:Make restclient read config only once

[jimma]

No description provided.

[asoldano]

@jimma the changes look ok to me, but I'm wondering if they go against what you're saying in https://issues.redhat.com/browse/RESTEASY-2525?focusedCommentId=14038622&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14038622 . Can you clarify please? Thanks

[ronsigal]

Hi @jimma,

I might be missing something.

1. I put a comment on the JIRA to try to understand just what Márk Petrényi is asking for.
2. It looks like getConfigProperties() is just called once when the client proxy is created, so I'm not sure how the readConfig guard helps.

??

[jimma]

@jimma the changes look ok to me, but I'm wondering if they go against what you're saying in https://issues.redhat.com/browse/RESTEASY-2525?focusedCommentId=14038622&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14038622 . Can you clarify please? Thanks

I checked this with @dependant annotation, and it looks we can still improve @Depandant scope to read only config once by this PR. I add this info to JIRA too.

[jimma]

Hi @jimma,

I might be missing something.

1. I put a comment on the JIRA to try to understand just what Márk Petrényi is asking for.
2. It looks like getConfigProperties() is just called once when the client proxy is created, so I'm not sure how the readConfig guard helps.

??

@ronsigal The rest client default scope is @dependant and default scope of resource is request scope, so each request will create this proxy along with a config read. From code debug, if there are all default setting as the , the config will be read for each resource method invocation.

[ronsigal]

Hey @jimma,

re: "the config will be read for each resource method invocation"

Yes, I agree. But I don't see how your update changes that.

Maybe if the flag and the map were static ... . I think that making them static would be safe, since the values come from sources that, at least for the standard ConfigSources, shouldn't change after deployment. I'm not sure if that's true for the Wildfly specific ConfigSources like a file of directory names, though.

In the question I posted on the JIRA, I'm wondering if Márk Petrényi is suggesting that, instead of looping through all of the names in all of the ConfigSources, it would be faster to look for the properties that are specific to MP REST Client.

-Ron

[jimma]

Hey @jimma,

re: "the config will be read for each resource method invocation"

Yes, I agree. But I don't see how your update changes that.

The flag will be checked to skip the properties read and directly return the result map when it's true.

Maybe if the flag and the map were static ... . I think that making them static would be safe, since the values come from sources that, at least for the standard ConfigSources, shouldn't change after deployment. I'm not sure if that's true for the Wildfly specific ConfigSources like a file of directory names, though.

if we have multiple deployments in container, will static get all deployments shares the same config values from the first read deployment because RestClientDelegateBean

In the question I posted on the JIRA, I'm wondering if Márk Petrényi is suggesting that, instead of looping through all of the names in all of the ConfigSources, it would be faster to look for the properties that are specific to MP REST Client.

Looks these properties are read and configured before getConfigProperties() : https://github.com/resteasy/Resteasy/blob/master/resteasy-client-microprofile/src/main/java/org/jboss/resteasy/microprofile/client/RestClientDelegateBean.java#L120~L126 ?

[ronsigal]

re: "The flag will be checked to skip the properties read and directly return the result map when it's true."

What am I missing? If the scope of the REST Client is @dependent, and it lives in a @request scoped resource, then getConfigProperties() would get called once for each invocation, right? So there won't be a second pass through getConfigProperties() when readConfig is true. ??

re: "Looks these properties are read and configured before getConfigProperties()"

Ah, yes, you're right. So getConfigProperties() is re-checking those properties, which is redundant. I haven't checkout which properties are not checked by the time we get to getConfigProperties(), but those, if any, could also be checked explicitly.

[jimma]

What am I missing? If the scope of the REST Client is @dependent, and it lives in a @request scoped resource, then getConfigProperties() would get called once for each invocation, right? So there won't be a second pass through getConfigProperties() when readConfig is true. ??

This is all true with this PR change.

Ah, yes, you're right. So getConfigProperties() is re-checking those properties, which is redundant. I haven't checkout which properties are not checked by the time we get to getConfigProperties(), but those, if any, could also be checked explicitly.

getConfigProperties() seems not redundant. It only reads the property with prefix like "myClient/property/" from mp-config.

[ronsigal]

Hi @jimma,

This is all true with this PR change.

But what I'm saying is that checking readConfig doesn't accomplish anything because there won't be a second time through getConfigProperties(). Is that not true?

It only reads the property with prefix like "myClient/property/" from mp-config.

it looks to me like

        for (String propertyName : config.getPropertyNames()) {
            if (propertyName.startsWith(property)) {
                Integer value = config.getValue(propertyName, Integer.class);
                String strippedProperty = propertyName.replace(property, "");
                configProperties.put(strippedProperty, value);
            }
        }

will loop through all of the properties (even if it doesn't look at them because of the if statement), including "myClient/mp-rest/url", "myClient/mp-rest/uri", etc., which have already been read. Moreover, that requires accessing all of the ConfigSources just to get the names, which I think is what Márk Petrényi is saying might be expensive.

[jimma]

But what I'm saying is that checking readConfig doesn't accomplish anything because there won't be a second time through getConfigProperties(). Is that not true?

From debug, by default @dependant scope creates client proxy and calls getConfigProperites() for each request:

    @Override
    public Object create(CreationalContext<Object> creationalContext) {
        RestClientBuilder builder = RestClientBuilder.newBuilder();

        configureUri(builder);

        configureTimeouts(builder);

        configureProviders(builder);

        configureSsl(builder);

        getConfigProperties().forEach(builder::property);
        return builder.build(proxyType);
    }

it looks to me like

        for (String propertyName : config.getPropertyNames()) {
            if (propertyName.startsWith(property)) {
                Integer value = config.getValue(propertyName, Integer.class);
                String strippedProperty = propertyName.replace(property, "");
                configProperties.put(strippedProperty, value);
            }
        }

will loop through all of the properties (even if it doesn't look at them because of the if statement), including "myClient/mp-rest/url", "myClient/mp-rest/uri", etc., which have already been read. Moreover, that requires accessing all of the ConfigSources just to get the names, which I think is what Márk Petrényi is saying might be expensive.

Ah. That is something we need to have a look. I don't know much about microprofile-config. Is there api we can leverage to filter out these properties already read ?

[ronsigal]

Hey @jimma,

From debug, by default @dependant scope creates client proxy and calls getConfigProperites() for each request:

I think I see where we're disagreeing. I just noticed (or noticed again, maybe) that RestClientDelegateBean is managed by CDI. Offhand, I can't remember how CDI handles a @dependent bean. Is it treated like a stateless EJB, where there are one or more backing instances that get reused? Or is it recreated each time? I've been thinking the latter, in which case
configProperties would get recreated each time. I should know this.

Is there api we can leverage to filter out these properties already read

I'm not sure. I'll take a look.

[jamezp]

Given the MicroProfile is moved to https://github.com/resteasy/resteasy-microprofile I'm closing this.