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

feat: add RFD describing proposed batched query feature #550

Merged
merged 1 commit into from
Nov 20, 2024

Conversation

j-lanson
Copy link
Collaborator

@j-lanson j-lanson commented Oct 30, 2024

Resolves #502 .

@j-lanson j-lanson added type: enhancement New feature or request product: hc Relates to the core "hc" binary product: rust-sdk Relates to the Rust plugin SDK labels Oct 30, 2024
@j-lanson j-lanson added this to the 3.9.0 milestone Oct 30, 2024
@j-lanson j-lanson self-assigned this Oct 30, 2024
@alilleybrinker
Copy link
Collaborator

Thanks for this! Two comments:

  • On the SDK changes, I think it should be possible to still have a single query function which uses a new trait to permit calling it with some a singular &'static str and &[&'static str] (or anything that derefs to that).
  • Could the submit chunking actually be split into a separate RFD? It can be a small one. I'd view this as an oversight in the original design, I just think putting it here may make it hard to find later.

Copy link
Collaborator

@alilleybrinker alilleybrinker left a comment

Choose a reason for hiding this comment

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

(Sorry, forgot to give my comments as a review: see my comments above)

@j-lanson
Copy link
Collaborator Author

  • On the SDK changes, I think it should be possible to still have a single query function which uses a new trait to permit calling it with some a singular &'static str and &[&'static str] (or anything that derefs to that).

Can you elaborate on this point? As I understand, you're suggesting not having an explicit batch_query() function. I think interpreting whether a user wants to batch or not dependent on the type of the passed variable is wrong, because there may be some query endpoints that still take a Vec of objects. query(Vec<Value>) means "send this as a single request to the endpoint", batch_query(Vec<Value>) means "I don't want to waste time sending hc core each value individually, but they each represent a different query to X plugin"

@alilleybrinker
Copy link
Collaborator

  • On the SDK changes, I think it should be possible to still have a single query function which uses a new trait to permit calling it with some a singular &'static str and &[&'static str] (or anything that derefs to that).

Can you elaborate on this point? As I understand, you're suggesting not having an explicit batch_query() function. I think interpreting whether a user wants to batch or not dependent on the type of the passed variable is wrong, because there may be some query endpoints that still take a Vec of objects. query(Vec<Value>) means "send this as a single request to the endpoint", batch_query(Vec<Value>) means "I don't want to waste time sending hc core each value individually, but they each represent a different query to X plugin"

Ah, you're right. Another thought though has occurred to me. It may be useful to, in addition to a singular batch_query method, also expose a batch() method which returns a handle with its own query() method, with the handle being consumed and the query sent with a send() (or some similar name) final method.

@j-lanson
Copy link
Collaborator Author

I added to the proposed PluginEngine interface changes to include the batch() function, and split the Submit chunking proposal into its own RFD

Copy link
Collaborator

@alilleybrinker alilleybrinker left a comment

Choose a reason for hiding this comment

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

LGTM! 🎉

@alilleybrinker alilleybrinker merged commit 814b1ee into main Nov 20, 2024
7 checks passed
@alilleybrinker alilleybrinker deleted the jlanson/batch-query-rfd branch November 22, 2024 18:30
@alilleybrinker alilleybrinker modified the milestones: 3.9.0, 3.8.0 Nov 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product: hc Relates to the core "hc" binary product: rust-sdk Relates to the Rust plugin SDK type: enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Investigate and write RFD for batching plugin queries
2 participants