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 a bug in HyperFeatureRequester::get_all() #65

Closed
wants to merge 2 commits into from

Conversation

aalexandrov
Copy link

The FeatureRequester::get_all() implementation for the HyperFeatureRequester is currently reading the entire response (as a test try a payload that is more than 8000 bytes long).

Requirements

  • I have added test coverage for new or changed functionality
  • I have followed the repository's pull request submission guidelines
  • I have validated my changes against all supported platform versions

Related issues

Provide links to any issues in this repository or elsewhere relating to this pull request.

Describe the solution you've provided

Provide a clear and concise description of what you expect to happen.

Describe alternatives you've considered

Provide a clear and concise description of any alternative solutions or features you've considered.

Additional context

Add any other context about the pull request here.

The `FeatureRequester::get_all()` implementation for the `HyperFeatureRequester` is currently reading the entire response (as a test try a payload that is more than 8000 bytes long).
@aalexandrov aalexandrov requested a review from a team March 13, 2024 10:47
@keelerm84
Copy link
Member

My apologies for the delay on this PR. There are a few other matters I still have to attend to this week, but I will try to get some resolution on this for you by the end of the week.

Thank you for your patience.

@aalexandrov
Copy link
Author

@keelerm84 It's not urgent on our side, it's just something I noticed when I was trying something out.

@keelerm84
Copy link
Member

@aalexandrov Thanks for letting me know this wasn't urgent as it gave me some breathing room. I appreciate that!

I was looking into this change this morning, and I was curious what issue you are trying to solve. You mention in the original write-up that I should:

as a test try a payload that is more than 8000 bytes long

with the default rust SDK version, this seems to work fine for my payload (~12k bytes). Were you experiencing a memory or CPU spike?

@aalexandrov
Copy link
Author

I think I was trying to create a mock API endpoint that only implements the regular HTTP protocol (no streaming API) and returns a sizeable JSON response. I wasn't able to make my client SDK consume the entire response without this change.

@keelerm84
Copy link
Member

Hmmm. I can't seem to reproduce this problem against our systems. I am using a payload that is around 17k bytes and it seems to work without issue.

Are you able to provide a reproduction case?

@keelerm84 keelerm84 added the waiting for feedback Indicates LaunchDarkly is waiting for customer feedback before issue is closed due to staleness. label Apr 10, 2024
@aalexandrov
Copy link
Author

I'm sorry, I think my mock Python server was on a WIP branch that has been lost.

@keelerm84
Copy link
Member

No worries at all. However, without a clear reproduction case, I'm going to close this PR. Please do not hesitate to re-open this issue if you are able to reproduce this error.

Thank you for all of the time and effort you have invested!

@keelerm84 keelerm84 closed this Apr 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting for feedback Indicates LaunchDarkly is waiting for customer feedback before issue is closed due to staleness.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants