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

implement failed strategy #77

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

Conversation

perama-v
Copy link
Contributor

@perama-v perama-v commented Mar 10, 2023

Issue addressed

Proposed changes

Adds the SelectionStrategy::Failed

Additional info

  • unit test for strategy

@pipermerriam
Copy link
Member

minor conflict to be cleaned up and I'll get this merged

Copy link
Member

@pipermerriam pipermerriam left a comment

Choose a reason for hiding this comment

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

I think the querying in this strategy is going to need a bit more machinery (most of which I'm inclined to have tested to be sure we're actually doing things correctly).

First, I worry about race conditions in the "queue". If I'm reading the code correctly, I think that this strategy will continue to select the same items if the rate the queue is being processed is slower than the rate this loop runs, which I believe results in the queue getting filled up with multiple copies of the same item.

Second, I am unsure if this is going to do the correct thing when a piece of content has multiple audits, some of which are failing and some of which are succeeding. Are we sure that it will only pick the ones who's most recent audit has failed? The code currently looks like it will filter out all successful audits, and then select the most recent failed one... which doesn't seem correct.

@perama-v perama-v marked this pull request as draft March 15, 2023 23:31
@perama-v
Copy link
Contributor Author

which I believe results in the queue getting filled up with multiple copies of the same item.

Agree with this observation. It will need to be addressed

The code currently looks like it will filter out all successful audits, and then select the most recent failed one... which doesn't seem correct.

Agree. Understanding how to effectively execute a subquery for the solution using sea-orm remains elusive to me.

@pipermerriam
Copy link
Member

Agree. Understanding how to effectively execute a subquery for the solution using sea-orm remains elusive to me.

Did you find this bit in the docs?

https://www.sea-ql.org/SeaORM/docs/advanced-query/subquery/

@perama-v
Copy link
Contributor Author

Did you find this bit in the docs?

I did. I just haven't been able to put it together to do the sequence of things required for the strategy

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.

2 participants