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

CV2-4719 add unique identifier as doc_id at check-api level #1926

Merged
merged 12 commits into from
Jun 24, 2024

Conversation

DGaffney
Copy link
Contributor

Description

On Presto, oftentimes we unnecessarily fingerprint the same item several times. With this change, we will plumb a content_hash key along through Check-API to Alegre to Presto, so that Presto can use the key to do an internal lookup against a Redis server to see if we have a recently fingerprinted item with that ID and just return it directly.

References: CV2-4719

How has this been tested?

No tests yet - expanding the data package will very likely cause many test errors, so this first change is to just highlight/record them.

Things to pay attention to during code review

Nothing significant at the Check-API stage, probably.

Checklist

  • I have performed a self-review of my own code
  • I have added unit and feature tests, if the PR implements a new feature or otherwise would benefit from additional testing
  • I have added regression tests, if the PR fixes a bug
  • I have added logging, exception reporting, and custom tracing with any additional information required for debugging
  • I considered secure coding practices when writing this code. Any security concerns are noted above.
  • I have commented my code in hard-to-understand areas, if any
  • I have made needed changes to the README
  • My changes generate no new warnings
  • If I added a third party module, I included a rationale for doing so and followed our current guidelines

@caiosba caiosba removed their request for review June 17, 2024 20:35
@caiosba caiosba requested review from caiosba and computermacgyver and removed request for melsawy June 20, 2024 13:16
Copy link
Contributor

@caiosba caiosba left a comment

Choose a reason for hiding this comment

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

Devin, please add the missing tests.

@DGaffney
Copy link
Contributor Author

@caiosba will do - the alegre_v2.rb missing lines are obvious, but no idea why bot_user.rb is complaining here, is that upstream of this ticket or am I just not seeing the tie?

app/models/concerns/alegre_v2.rb: 153, 156, 159, 160
app/models/bot_user.rb: 90, 136, 137, 147, 148, 149, 150, 151, 152, 153, 154, 155, 156, 157, 158, 159, 160, 161, 194, 195, 196, 197, 198, 199, 226, 230, 234, 235, 239, 240, 241, 242, 243, 244, 245, 247, 334, 344, 475

@caiosba
Copy link
Contributor

caiosba commented Jun 20, 2024

I think that the BotUser ones are flaky (or not covered because of the other failures).

@DGaffney
Copy link
Contributor Author

@caiosba sounds good - I'll fix the others first, we'll rerun, then assess

headers
)
end

Copy link

Choose a reason for hiding this comment

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

Trailing whitespace detected.

@DGaffney DGaffney requested a review from caiosba June 21, 2024 22:20
app/models/concerns/smooch_search.rb Outdated Show resolved Hide resolved
app/models/concerns/smooch_search.rb Outdated Show resolved Hide resolved
Copy link

codeclimate bot commented Jun 22, 2024

Code Climate has analyzed commit e295099 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (100% is the threshold).

This pull request will bring the total coverage in the repository to 99.9% (0.0% change).

View more on Code Climate.

@caiosba caiosba self-requested a review June 22, 2024 21:18
@DGaffney DGaffney merged commit 95c4af9 into develop Jun 24, 2024
8 checks passed
@DGaffney DGaffney deleted the cv2-4719-presto-caching branch June 24, 2024 14:43
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.

3 participants