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(issue-platform): remove +1 on limit #77115

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

JoshFerge
Copy link
Member

Fixes SENTRY-3D61

its unclear to me why we'd add a +1 onto our LIMIT. we should be able to handle limit=10000 like our other query strategies, but adding plus 1 makes it 10001 which causes it to error.

let's see if any tests break because of this

@JoshFerge JoshFerge requested a review from a team as a code owner September 6, 2024 23:10
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Sep 6, 2024
Copy link

codecov bot commented Sep 6, 2024

❌ 2 Tests Failed:

Tests completed Failed Passed Skipped
21670 2 21668 206
View the top 2 failed tests by shortest run time
tests.sentry.issues.endpoints.test_organization_group_index.GroupListTest test_simple_pagination
Stack Traces | 9.52s run time
#x1B[1m#x1B[.../issues/endpoints/test_organization_group_index.py#x1B[0m:429: in test_simple_pagination
    assert links["next"]["results"] == "true"
#x1B[1m#x1B[31mE   AssertionError: assert 'false' == 'true'#x1B[0m
#x1B[1m#x1B[31mE     #x1B[0m
#x1B[1m#x1B[31mE     - true#x1B[0m
#x1B[1m#x1B[31mE     + false#x1B[0m
tests.sentry.issues.endpoints.test_organization_group_index.GroupListTest test_pagination_and_x_hits_header
Stack Traces | 43.1s run time
#x1B[1m#x1B[.../issues/endpoints/test_organization_group_index.py#x1B[0m:3307: in test_pagination_and_x_hits_header
    assert next_obj["results"] == "true"
#x1B[1m#x1B[31mE   AssertionError: assert 'false' == 'true'#x1B[0m
#x1B[1m#x1B[31mE     #x1B[0m
#x1B[1m#x1B[31mE     - true#x1B[0m
#x1B[1m#x1B[31mE     + false#x1B[0m

To view individual test run time comparison to the main branch, go to the Test Analytics Dashboard

Copy link
Member

@wedamija wedamija left a comment

Choose a reason for hiding this comment

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

This is usually for pagination, so that we can see if there's another page to fetch. Are you sure removing this won't break anything?

@JoshFerge
Copy link
Member Author

This is usually for pagination, so that we can see if there's another page to fetch. Are you sure removing this won't break anything?

seems like it did break a few tests. was just trying to see. that makes sense. its hard as it seems like the limit in our API is 10,000, but the snuba limit is also 10,000, so the +1 here is causing lots of errors from clients. guess we need to make the limit 9,999 instead?

@wedamija
Copy link
Member

wedamija commented Sep 9, 2024

This is usually for pagination, so that we can see if there's another page to fetch. Are you sure removing this won't break anything?

seems like it did break a few tests. was just trying to see. that makes sense. its hard as it seems like the limit in our API is 10,000, but the snuba limit is also 10,000, so the +1 here is causing lots of errors from clients. guess we need to make the limit 9,999 instead?

Yeah, that seems fine to me

@getsantry
Copy link
Contributor

getsantry bot commented Oct 1, 2024

This pull request has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you add the label WIP, I will leave it alone unless WIP is removed ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

@getsantry getsantry bot added Stale and removed Stale labels Oct 1, 2024
@getsantry
Copy link
Contributor

getsantry bot commented Oct 24, 2024

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you remove the label Waiting for: Community, I will leave it alone ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

@getsantry getsantry bot added Stale and removed Stale labels Oct 24, 2024
@JoshFerge
Copy link
Member Author

https://sentry.sentry.io/issues/5683689835/ is still spiking. worth trying to merge a fix for this

@getsantry
Copy link
Contributor

getsantry bot commented Nov 20, 2024

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you remove the label Waiting for: Community, I will leave it alone ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

@getsantry getsantry bot added Stale and removed Stale labels Nov 20, 2024
@getsantry
Copy link
Contributor

getsantry bot commented Dec 13, 2024

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you remove the label Waiting for: Community, I will leave it alone ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

@getsantry getsantry bot added Stale and removed Stale labels Dec 13, 2024
@getsantry
Copy link
Contributor

getsantry bot commented Jan 5, 2025

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you remove the label Waiting for: Community, I will leave it alone ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

@getsantry getsantry bot added Stale and removed Stale labels Jan 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants