search

flightplan-tool / flightplan

Search for award inventory using Node.js and Headless Chrome!
Apache License 2.0
142 stars 41 forks source link

Before performing a search, we are now cleaning up any existing awards #49

Closed zubairv85 closed 5 years ago

zubairv85 commented 5 years ago

When performing a search, we do not remove any existing awards from the database for the same exact search. This leads to scenarios where you find award space one day, and then do the same search the next day when there are no awards available yet the flightplan website still shows space available.

jd20 commented 5 years ago

Not sure I understand the motivation behind this change... seems like you'd be breaking the way results are cached in the database. By design, the search command doesn't overwrite existing results, it simply fills in empty gaps. That way, if I'm running a search for an entire year, and the command gets interrupted, I can always just re-run the same command, and it will pick up where it left off (plus if some days failed, it will go back and fill those gaps in).

If you want the behavior you're describing, just add --force, and it will re-run all searches regardless of whether it exists in the database already or not. If you want to get more fine-grained, for example, remove any cached results older than 12 hours, then run flightplan cleanup --maxage PT12H. Is there another use case, that wouldn't be covered by what --force and the cleanup command allow?

zubairv85 commented 5 years ago

The scenario I was coding against was when you do an award search for a particular day and find space. Then the next day you do the exact same search and don't find any space. The previous results are still in the DB so the flight plan app shows that there is space when there isn't.

I tried the force but that doesn't clear up the DB entries so the space still shows. I did a cleanup but that seemed to have just removed all of the requests and kept the awards table in tact. I didn't try doing a cleanup with the maxage property though. That might be what I am looking for. Let me try it again and if that does what I need I will cancel the PR.

Thanks!

Zubair Valimohideen

On Sat, Jun 8, 2019, 7:15 PM Jason notifications@github.com wrote:

Not sure I understand the motivation behind this change... seems like you'd be breaking the way results are cached in the database. By design, the search command doesn't overwrite existing results, it simply fills in empty gaps. That way, if I'm running a search for an entire year, and the command gets interrupted, I can always just re-run the same command, and it will pick up where it left off (plus if some days failed, it will go back and fill those gaps in).

If you want the behavior you're describing, just add --force, and it will re-run all searches regardless of whether it exists in the database already or not. If you want to get more fine-grained, for example, remove any cached results older than 12 hours, then run flightplan cleanup --maxage PT12H. Is there another use case, that wouldn't be covered by what --force and the cleanup command allow?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/flightplan-tool/flightplan/pull/49?email_source=notifications&email_token=AAEXPWP7NRM5XFQW3XS56TLPZRDTTA5CNFSM4HWECAE2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXIA7EA#issuecomment-500174736, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEXPWMIOBOXQZY6JUZZ4TDPZRDTTANCNFSM4HWECAEQ .

zubairv85 commented 5 years ago

It looks like the cleanup only removes requests older than a specified date. The awards are still there and so they still show up in the app when searching for award space. So it doesnt look like force or the cleanup accomplish the cleanup of the award entries.

It looks like the cli-parse.js will do what I am trying to accomplish by deleting all awards for each request and reparsing the cached data. However, since I have already run the cleanup, I dont have any requests in my DB to reparse so its not a reliable solution.

How would you feel about keeping the PR changes but putting them under an optional flag?

jd20 commented 5 years ago

Yeah, the real problem is that we're not clearing old awards, when we write awards for a new request. Instead, those old awards don't get removed until the old request is deleted, which may never happen.

The way I would fix this, is at the very beginning of helpers.saveAwards, query for all awards that would be "covered" by the request (similar to how you are doing now), and delete them. This way, the old awards stay intact in the database, until we actually are ready to write the new awards (and will overwrite old awards even if they belong to a request that no longer exists). It also ensures that things work properly if the --no-parser flag is being used (the logic needs to be invoked anywhere we write to the awards table, which is scoped to helpers.saveAwards. Done that way, there shouldn't be any adverse side effects, and it'll fix some lingering bugs.

zubairv85 commented 5 years ago

That's a better implementation. I'll update the PR with these changes.