-
Notifications
You must be signed in to change notification settings - Fork 51
feat(flagd): add http connector for In-process resolver #1299
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
base: main
Are you sure you want to change the base?
feat(flagd): add http connector for In-process resolver #1299
Conversation
Signed-off-by: liran2000 <[email protected]>
Signed-off-by: liran2000 <[email protected]>
Signed-off-by: liran2000 <[email protected]>
Signed-off-by: liran2000 <[email protected]>
Signed-off-by: liran2000 <[email protected]>
To be honest, I am a little bit torn by this request. I like the option of a custom connector, and I think an HTTP connector is a nice feature to offer. But the flagd provider is already complex and has much additional logic. Should we maybe start extracting this custom connector into own artifacts? In this case, the flagd provider code won't gain more complexity, the deliverable size is smaller, and people can opt-in with an additional package. It might be worth talking about the general structure of flagd and maybe extracting all connection modes into their connectors (out of scope here) - but it might be worth starting with the HTTP connector. wdyt? |
Understood. One of the main need of this is to reduce infrastructure/devops work, without additional containers needed. At some dev teams, such things can be a blocker for adopting new components, as without dependency on infrastructure/devops work, dev teams can add it independently, on dev repositories alone. Regarding how to add it, possibly as some separate extension, similar to junit-open-feature-extension ? |
This sounds like a reasonable solution and the setup here would be similar. but currently this is just my opinion, it would be great to hear what overs think about this topic, before we proceed. @toddbaert @chrfwow @open-feature/sdk-java-maintainers (not sure whom to tag, as this is just a provider, and not the basic SDK, not sure if this is relevant for the tc) |
Sounds reasonable to me to make flagd providers extensible. Like we have different sync sources for flagd itself, we could also pull this concept down to the provider level. What I'm wondering is how we would manage alignment of these sources between different provider implementations. Every source protocol added should ideally be available across provider implementations in different languages. I would see the basic logic to connect to flagd as core part of flagd providers. But something like the |
I can see why people would want this! Like @aepfli , my main concern is maintenance burden. We already have a lot of feature and configuration disparity between flagd providers across languages, and I don't want to add more before we've even resolved what's already a problem. Similar to what @guidobrei said, my recommendation would be that we keep all the parts of this PR that allow people to do this sort of extension easily themselves (defining interfaces, etc) so that somebody could make an http-connector (or an s3 connector, etc, etc). Then @liran2000 you could host the HTTP connector yourself if you'd like, or possibly as a separate component in contribs. Right now we maintain these sorts of things in flagd itself so people can use http/s3 and we just have that one repo to maintain. Implementing such things in every flagd provider could mean possibly hundreds of additional modules, and I simply don't think we have the maintainership resources for that. |
Signed-off-by: liran2000 <[email protected]>
…to feature/flagd-http-connector
Signed-off-by: liran2000 <[email protected]>
Signed-off-by: liran2000 <[email protected]>
Signed-off-by: liran2000 <[email protected]>
Can you have a look on the new structure, located under tools, what do you think ? |
} | ||
log.debug("adding payload to queue"); | ||
if (!this.queue.offer(new QueuePayload(QueuePayloadType.DATA, payload))) { | ||
log.warn("Unable to offer file content to queue: queue is full"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't we purge the queue or at least remove the oldest element? Getting new data sounds more important to me thatn keeping the old one
} | ||
|
||
public void updatePayloadIfNeeded(String payload) { | ||
if ((getCurrentTimeMillis() - lastUpdateTimeMs) < updateIntervalMs) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand why we would want to ignore updates to the cache. I understand why we wouldn't want to make requests before the update interval runs out to save resources. But if we happen to make a request and get updated data, why would we ignore it?
add http connector for In-process resolver.