-
Notifications
You must be signed in to change notification settings - Fork 10
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
fix(xp-treatment): Add field tags to poller configs and make XP plugin pass poller configs #88
Merged
deadlycoconuts
merged 9 commits into
caraml-dev:main
from
deadlycoconuts:add_json_field_tags_for_poller_configs
Jan 20, 2025
Merged
fix(xp-treatment): Add field tags to poller configs and make XP plugin pass poller configs #88
deadlycoconuts
merged 9 commits into
caraml-dev:main
from
deadlycoconuts:add_json_field_tags_for_poller_configs
Jan 20, 2025
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
deadlycoconuts
requested review from
mbruner,
shydefoo,
bthari and
vinoth-gojek
January 16, 2025 08:10
vinoth-gojek
approved these changes
Jan 16, 2025
deadlycoconuts
commented
Jan 17, 2025
deadlycoconuts
commented
Jan 17, 2025
deadlycoconuts
commented
Jan 17, 2025
deadlycoconuts
commented
Jan 17, 2025
Thanks a lot for the review! :D Merging this now! 🚀 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
In PR #87, a new XP Management Service poller was introduced to the XP Treatment Service. Unfortunately, this feature is not usable within the XP plugin for a couple of reasons, which this PR seeks to address:
ManagementServicePollerConfig
config struct required for this feature lacks the necessary field tags (json
) to parse configs from the config file into actual values in Gojson
tags to the fields ofManagementServicePollerConfig
ManagementServicePollerConfig.PollInterval
field is of thetime.Duration
type, which doesn't allow yaml/json values (this is how the XP configs are configured, passed between various components) to be easily parsed into validtime.Duration
valuesPollInterval
field to theint
data typeManagementServicePollerConfig
field to the plugin config structTreatmentServicePluginConfig
which needs to be configured on the Turing API server (this config gets passed to the XP plugin whenever a Turing router is deployed)Server
, which isn't initialised/used within the XP runner pluginPoller
object of theserver
package into aPollerService
of theservices
package that gets initialised and started up in both the XP Treatment Service and XP plugin runnerAdditional Changes to the XP Plugin
This PR also updates the way the XP plugin manager retrieves the
MessageQueueConfig
(and then passes it to the XP plugin runner). Previously this config is retrieved from the XP Management service, which strongly couples the way the Management Service publishes experiment updates with the way the XP plugin runner retrieves these updates i.e. if the XP Management Service uses PubSub to publish updates, the XP plugin runner MUST also use PubSub to receive those updates.With this new change, the
MessageQueueConfig
must now be specified explicitly as part of the XP plugin manager configs (specified in the Turing API server's experiment engine configs). This allows the XP plugin to receive experiment updates in a way that's independent of the XP Management Service e.g. routers using the XP plugin can use the new poller service to retrieve updates from the Management Service even while the Management Service continues to publish updates to PubSub (presumably for other XP plugins/Treatment Service instances to receive updates via PubSub).Note that the current implementation DOES NOT clean up the XP Management Service to stop serving the
MessageQueueConfig
via one of its API endpoints. This will be done at a later time to clean up its current implementation.cc @rautelaanirudh
Which issue(s) this PR fixes:
See above