Skip to content

Conversation

@dennishuo
Copy link
Contributor

@dennishuo dennishuo commented Sep 26, 2024

Description

At 10 per second, PolarisRestCatalogIntegrationTest takes about 2m50s. At 20 per second, it takes about 1m25s.
At 50 per second, it takes about 30s.
At 100 per second it takes about 19s.
At 200 per second it takes about 19s.

Type of change

Please delete options that are not relevant.

  • Test only

Checklist:

Please delete options that are not relevant.

  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • If adding new functionality, I have discussed my implementation with the community using the linked GitHub issue

At 10 per second, PolarisRestCatalogIntegrationTest takes about 2m50s.
At 20 per second, it takes about 1m25s.
At 50 per second, it takes about 30s.
At 100 per second it takes about 19s.
At 200 per second it takes about 19s.
eric-maynard
eric-maynard previously approved these changes Sep 26, 2024
Copy link
Contributor

@eric-maynard eric-maynard left a comment

Choose a reason for hiding this comment

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

LGTM, maybe we should go even higher than 100.

If we are not intending to test the rate limiter I would rather turn it off or go to 9999+

@dennishuo
Copy link
Contributor Author

Good suggestion, bumped up to 9999. In theory 100 might "accidentally" exercise the backoff once in awhile, but thinking about it more we should be more explicit if we really want to exercise the actual 429 errors anyways, and it'd be fragile to just balance the limit at some number just on the boundary, since tests change over time.

I think it's still reasonable to keep the rate limiter in the mix to exercise at least the initialization and "happy path" of not hitting limits for these "end-to-end integration tests" so that all components are there working together.

9999 should be good for being a not-infinite but effectively infinite limit given the round-trip overhead of these end-to-end calls aren't expected to be able to do more than 10 per millisecond anyways.

@dennishuo dennishuo changed the title Bump up the default integration-test requests-per-second to 100 vs 10 Bump up the default integration-test requests-per-second to 9999 vs 10 Sep 26, 2024
@jbonofre jbonofre merged commit 2897160 into apache:main Sep 26, 2024
@dennishuo dennishuo deleted the dhuo-relax-rate-limit-for-integration-tests branch February 14, 2025 03:37
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.

4 participants