spring-cloud-config icon indicating copy to clipboard operation
spring-cloud-config copied to clipboard

Enable force refresh by forceRefresh query parameter

Open opeco17 opened this issue 10 months ago • 9 comments

Fixes https://github.com/spring-cloud/spring-cloud-config/issues/2401

Force refresh can be enabled when both of the following are true.

  1. allowForceRefresh is true in configuration of Config Server
  2. forceRefresh query parameter is true like http://localhost:8888/foo-default.yml?forceRefresh=true

opeco17 avatar Apr 07 '24 04:04 opeco17

This would be a good enhancement for our next major release where we can introduce some breaking changes. We think it would be to explore introducing a "context object" so that next time we need to pass a parameter like this down the stack we aren't changing a bunch of method signatures to do so.

ryanjbaxter avatar Apr 10 '24 18:04 ryanjbaxter

@ryanjbaxter

Thank you for the review. May I know which approach you prefer regarding context implementation?

Approach A (Example)

public class RequestContext {
    private static final ThreadLocal<Map<String, String>> context = new ThreadLocal<>();

    public static void setContext(Map<String, String> value) {
        context.set(value);
    }

    public static Map<String, String> getContext() {
        return context.get();
    }

    public static void clear() {
        context.remove();
    }
}
public class Service {
    public void doSomething() {
        String forceRefreshStr = RequestContext.get("forceRefresh");
        boolean forceRefresh = false;
        if (forceRefreshStr != null) {
            boolean forceRefres = Boolean.parseBoolean(forceRefreshStr);
        }
        // do something
    }
}

Approach B (Example)

public class RequestContext {
    private boolean forceRefresh;

    public RequestContext(boolean forceRefresh) {
        this.forceRefresh = forceRefresh;
    };

    public boolean getForceRefresh() {
        return forceRefresh;
    }
}
public class Service {
    public void doSomething(RequestContext ctx) {
        boolean forceRefresh = ctx.getForceRefresh();
        // do something
    }
}

opeco17 avatar Apr 11 '24 12:04 opeco17

Personally I would prefer approach B, but I would remove the constructor and just use setters and getters.

ryanjbaxter avatar Apr 11 '24 13:04 ryanjbaxter

@ryanjbaxter

I've implemented RequestContext to pass query parameters to downstream methods. When we introduce new query parameters that need to be passed to downstream methods like forceRefresh next time, we can add them to RequestContext.

opeco17 avatar Apr 13 '24 04:04 opeco17

@spencergibb

Should we add both query parameters and path parameters to RequestContext or just query parameters?

In my opinion, adding just query parameters is better because adding path parameters requires huge change. Once I've added just query parameters to RequestContext.

opeco17 avatar Apr 13 '24 06:04 opeco17

I think we would want query and path parameters (name, profile, label, etc).

And I agree it is a big changes, hence why it makes sense for a major release. IMO I tink the request context change should be separated out into its own PR.

ryanjbaxter avatar Apr 16 '24 14:04 ryanjbaxter

@ryanjbaxter

I agree with you

What about to add just query parameters to RequestContext in this PR and work on other parameters in another PR? Or it's also reasonable to remove RequestContext from this PR completely and introduce RequestContext in another PR in my opinion

I can work on another PR to complete RequestContext as quickly as possible once the direction is decided

opeco17 avatar Apr 17 '24 03:04 opeco17

I would't remove RequestContext from this PR. What I would suggest is we introduce RequestContext and move the existing parameters to it in another PR and then in this PR add forceRefresh to RequestContext. We would need to merge the first PR before merging this PR.

ryanjbaxter avatar Apr 17 '24 14:04 ryanjbaxter

@ryanjbaxter @spencergibb

I've created another PR to move the existing parameters to RequestContext. Could you please review https://github.com/spring-cloud/spring-cloud-config/pull/2408?

opeco17 avatar Apr 22 '24 13:04 opeco17