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

Fix the return order of batched requests #2114

Merged
merged 1 commit into from
Feb 13, 2024

Conversation

IMax153
Copy link
Member

@IMax153 IMax153 commented Feb 13, 2024

Type

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update

Description

Related

  • Related Issue #
  • Closes #

Copy link

changeset-bot bot commented Feb 13, 2024

🦋 Changeset detected

Latest commit: 616b2b9

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 15 packages
Name Type
effect Patch
@effect/cli Patch
@effect/experimental Patch
@effect/opentelemetry Patch
@effect/platform-browser Patch
@effect/platform-bun Patch
@effect/platform-node-shared Patch
@effect/platform-node Patch
@effect/platform Patch
@effect/printer-ansi Patch
@effect/printer Patch
@effect/rpc-http Patch
@effect/rpc Patch
@effect/schema Patch
@effect/typeclass Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@tim-smart
Copy link
Member

While it works and looks good to me, I'm not sure why the behaviour changed after Mike's request refactor.

I'll defer to @mikearnaldi to review for this one :)

@mikearnaldi
Copy link
Member

I don't think I understand the problem and by consequence the solution, would you mind writing a test?

@mikearnaldi
Copy link
Member

While it works and looks good to me, I'm not sure why the behaviour changed after Mike's request refactor.

I'll defer to @mikearnaldi to review for this one :)

I didn't touch that function

@IMax153
Copy link
Member Author

IMax153 commented Feb 13, 2024

While it works and looks good to me, I'm not sure why the behaviour changed after Mike's request refactor.
I'll defer to @mikearnaldi to review for this one :)

I didn't touch that function

I was pretty convinced this was my error in the original port :) We just probably never noticed the issue before.

@IMax153 IMax153 merged commit b881365 into main Feb 13, 2024
12 checks passed
@IMax153 IMax153 deleted the fix/parallel-collection-ordering branch February 13, 2024 14:28
@github-actions github-actions bot mentioned this pull request Feb 13, 2024
@IMax153
Copy link
Member Author

IMax153 commented Feb 13, 2024

The issue basically was - if you send a set of batched requests that were received as follows:

Request 1 -> Request 2 -> Request 3

the results were being returned as:

Result 3 -> Result 2 -> Result 1

We tracked down the inadvertent reversal of results to this method which was combining results in reverse order instead of the correct order.

I'll write a test in a separate PR - apologies for merging before seeing your messages.

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

Successfully merging this pull request may close these issues.

None yet

3 participants