Skip to content

ex-3615-add-filtering-by-cloud-event-id-to-fetch-api#28

Merged
Allyson-English merged 2 commits intomainfrom
ex-3615-add-filtering-by-cloud-event-id-to-fetch-api-B
May 20, 2025
Merged

ex-3615-add-filtering-by-cloud-event-id-to-fetch-api#28
Allyson-English merged 2 commits intomainfrom
ex-3615-add-filtering-by-cloud-event-id-to-fetch-api-B

Conversation

@Allyson-English
Copy link
Copy Markdown
Contributor

No description provided.

@linear
Copy link
Copy Markdown

linear Bot commented May 16, 2025

Comment thread internal/fetch/rpc/rpc.go
Source: getStringValue(protoOptions.GetSource()),
Producer: getStringValue(protoOptions.GetProducer()),
Extras: getStringValue(protoOptions.GetExtras()),
ID: getStringValue(protoOptions.GetId()),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm a little confused about what actually happens to this option. Where does it get processed?

Comment thread pkg/grpc/fetch-api.proto
google.protobuf.StringValue extras = 10;

// ID of the event to filter.
google.protobuf.StringValue id = 11;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Two things to think about:

  1. Do you want to let people pull multiple documents at once? That is, is it good for this to be a list?
  2. Do we want to require people to specify source if they do choose to specify an id? My sense is that ids are just not meaningful across sources.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah, I think you're following the existing pattern. I guess this is fine.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah this is something I want to go back and add to all parameters someday we just have yet to have a need for it.

@Allyson-English Allyson-English merged commit 51bd8cd into main May 20, 2025
3 checks passed
@Allyson-English Allyson-English deleted the ex-3615-add-filtering-by-cloud-event-id-to-fetch-api-B branch May 20, 2025 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants