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: dedupe dedupe settings #2183

Merged
merged 5 commits into from
Jun 14, 2024

Conversation

bnchi
Copy link
Contributor

@bnchi bnchi commented Jun 13, 2024

Summary:

  • Removes batch_execution field from Server config, dedupe_in_flight and dedupe fields from Upstream.
  • Adds dedupe to Server config
  • Refactor request_handler.rs and io.rs to use the new dedupe defined in Server config

Issue Reference(s):
Fixes #2182

Build & Testing:

  • I ran cargo test successfully.
  • I have run ./lint.sh --mode=fix to fix all linting issues raised by ./lint.sh --mode=check.

Checklist:

  • I have added relevant unit & integration tests.
  • I have updated the documentation accordingly.
  • I have performed a self-review of my code.
  • PR follows the naming convention of <type>(<optional scope>): <title>

@github-actions github-actions bot added the type: chore Routine tasks like conversions, reorganization, and maintenance work. label Jun 13, 2024
Copy link

codecov bot commented Jun 13, 2024

Codecov Report

Attention: Patch coverage is 86.66667% with 2 lines in your changes missing coverage. Please review.

Project coverage is 84.12%. Comparing base (dd098ca) to head (0740a43).

Files Patch % Lines
src/core/ir/io.rs 83.33% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2183      +/-   ##
==========================================
- Coverage   84.12%   84.12%   -0.01%     
==========================================
  Files         212      212              
  Lines       20054    20032      -22     
==========================================
- Hits        16870    16851      -19     
+ Misses       3184     3181       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tusharmath tusharmath changed the title chore: Restructure dedupe settings fix: dedupe dedupe settings Jun 13, 2024
@tusharmath tusharmath added the type: fix Iterations on existing features or infrastructure. label Jun 13, 2024
@tusharmath tusharmath enabled auto-merge (squash) June 13, 2024 15:06
@tusharmath tusharmath removed the type: chore Routine tasks like conversions, reorganization, and maintenance work. label Jun 13, 2024
@bnchi
Copy link
Contributor Author

bnchi commented Jun 13, 2024

Thanks for fixing the lint issues, I didn't expect the warning will cause the lint to fail in CI.

One more thing I wanted to quickly get your opinion on this line

if ctx.request_ctx.server.dedupe {

The above if - else block is probably not needed because this block will get executed only if this check here didn't pass

if !ctx.request_ctx.server.dedupe || !ctx.is_query() {

so I guess we can get rid of the if at line 74 completely right ?

The tests are still working on my machine if I removed that check.

@tusharmath

@tusharmath
Copy link
Contributor

That would be right.

auto-merge was automatically disabled June 13, 2024 18:19

Head branch was pushed to by a user without write access

@bnchi
Copy link
Contributor Author

bnchi commented Jun 13, 2024

I made a commit that removes the unnecessary check thanks.

@tusharmath
Copy link
Contributor

@bnchi Can you please update the documentation for these changes here - https://github.com/tailcallhq/tailcallhq.github.io

@tusharmath tusharmath merged commit 55070c2 into tailcallhq:main Jun 14, 2024
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: fix Iterations on existing features or infrastructure.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Restructure dedupe settings
2 participants