Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable force refresh by forceRefresh query parameter #2402

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

opeco17
Copy link

@opeco17 opeco17 commented Apr 7, 2024

Fixes #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

@ryanjbaxter
Copy link
Contributor

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.

@opeco17
Copy link
Author

opeco17 commented Apr 11, 2024

@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
    }
}

@ryanjbaxter
Copy link
Contributor

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

@opeco17
Copy link
Author

opeco17 commented Apr 13, 2024

@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.

Copy link
Member

@spencergibb spencergibb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Request context should include more than just force refresh. As many as possible, if not all of them

@opeco17
Copy link
Author

opeco17 commented Apr 13, 2024

@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 opeco17 force-pushed the feature/enable-force-refresh-by-query-parameter branch from ff91175 to 5da9072 Compare April 13, 2024 07:41
@ryanjbaxter
Copy link
Contributor

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.

@opeco17
Copy link
Author

opeco17 commented Apr 17, 2024

@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

@ryanjbaxter
Copy link
Contributor

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.

@opeco17
Copy link
Author

opeco17 commented Apr 22, 2024

@ryanjbaxter @spencergibb

I've created another PR to move the existing parameters to RequestContext.
Could you please review #2408?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable force refresh by forceRefresh query parameter regardless of refreshRate
4 participants