Skip to content

Go: Batch #3938

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

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

Go: Batch #3938

wants to merge 13 commits into from

Conversation

Yury-Fridlyand
Copy link
Collaborator

@Yury-Fridlyand Yury-Fridlyand commented May 21, 2025

Rust/FFI code copied and adopted from C# client.

Issue link

This Pull Request is linked to issue (URL): closes #3547, #2791

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one issue.
  • Commit message has a detailed description of what changed and why.
  • Tests are added or updated.
  • CHANGELOG.md and documentation files are updated.
  • Destination branch is correct - main or release
  • Commits will be squashed upon merging.

Signed-off-by: Yury-Fridlyand <[email protected]>
Signed-off-by: Yury-Fridlyand <[email protected]>
@Yury-Fridlyand Yury-Fridlyand added the go golang wrapper label May 21, 2025
@Yury-Fridlyand Yury-Fridlyand mentioned this pull request May 21, 2025
6 tasks
Signed-off-by: Yury-Fridlyand <[email protected]>
Signed-off-by: Yury-Fridlyand <[email protected]>
@Yury-Fridlyand Yury-Fridlyand marked this pull request as ready for review May 22, 2025 00:21
@Yury-Fridlyand Yury-Fridlyand requested a review from a team as a code owner May 22, 2025 00:21
Copy link
Collaborator

@yipin-chen yipin-chen left a comment

Choose a reason for hiding this comment

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

LGTM for non-ffi changes.

@yipin-chen yipin-chen mentioned this pull request May 22, 2025
34 tasks
Signed-off-by: Yury-Fridlyand <[email protected]>
Signed-off-by: Yury-Fridlyand <[email protected]>
Signed-off-by: Yury-Fridlyand <[email protected]>
Co-authored-by: Edward Liang <[email protected]>
Signed-off-by: Yury-Fridlyand <[email protected]>
Signed-off-by: Yury-Fridlyand <[email protected]>
Signed-off-by: Yury-Fridlyand <[email protected]>
Signed-off-by: Yury-Fridlyand <[email protected]>
delete(client.pending, resultChannelPtr)
}
client.mu.Unlock()
return nil, ctx.Err()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
return nil, ctx.Err()
// TODO: Need to deal with cleaning up any allocated memory
return nil, ctx.Err()


// ====================

// TODO - move this struct and convert methods to internals
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this is moved to internal, how would anyone use this Option? Why not just define this as batchOptions which would prevent creating the type, while still allowing the use of the type.


// ====================

func (b Batch) Convert(response []any) ([]any, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Documentation?

Copy link
Collaborator

@jbrinkman jbrinkman left a comment

Choose a reason for hiding this comment

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

Approved with comments :shipit:

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

Successfully merging this pull request may close these issues.

[Task] Go: add pipelines / batches wrapper support
5 participants