Add `retryAfter` to Bedrock::Jobs::GetJob about bedrock HOT 21 CLOSED

I can't remember why I wrote that. I can't think of a good explanation for that, so lacking a reason I suggest we ignore that comment.

from bedrock.

@danielk1977 - As the resident query plan master, can you comment on whether you think the above query plan would be significantly slower than the original? Thanks!

from bedrock.

They're quite similar. The first scans a subset of the index jobsStateNextRunName and does a by-rowid lookup on the main table for each index row scanned. It then tests the GLOB condition, and if it matches inserts the row into the temporary b-tree. Once it has accumulated all matching rows in the temp b-tree, it reads them out in sorted order to return to the user.

The second query is similar, except that it scans two regions of the index to collect rows.

I think the relative speed of these two queries will be roughly proportional to the number of rows returned. i.e. if the second returns twice as many rows as the first it will likely run in roughly twice the time.

Dan.

from bedrock.

from bedrock.

Actually, strike that -- it's actually super fast, it looks like a flaw in our monitoring system was mis-attributing a slowdown to Bedrock.

from bedrock.

So what's the status with this?
The PR was reverted with #194 - correct?

from bedrock.

Bump, any news on this?
If the code itself was fine and we only had to revert because of the schema changes taking too long when done via upgradeDatabase can we please look into manually running the schema changes first?

from bedrock.

We split the change into a schema change and a code change so that we could test them individually, and we're waiting for the schema change to be deployed. The fires this week have kept #infra pretty busy so it's been a bit delayed. When the schema change goes out, we'll see if that causes a problem, indicating a code problem, or if the schema change causes a problem itself.

from bedrock.

@coleaeason - we're still waiting on the schema change to be deployed, correct?

from bedrock.

I can help with the deploys, I'm down to deploy bedrock this week.

from bedrock.

Sorry, yeah i've had no time for this and it's pretty low priority for me compared to other things on my plate. If someone else ( @flodnv or @iwiznia ) wants to take ownership of deploying this since they are more invested in getting it out, then I'm fine with that.

from bedrock.

Bumping this again; I know you are having lots of other priorities, this would definitely help us on the Chatbot front though.

from bedrock.

@tylerkaraszewski where's the PR for this? Is is this #235 ? Then bump @cead22 @mea36

from bedrock.

@flodnv yes.

@fnwbr @mea36 feel free to take over recreating the PR. It's filled with conflicts that since the PRs CancelJob and CreateJobs got merged.

Please make sure to run the tests in #235 #191 and #195 when submitting the PR
cc @pecanoro

from bedrock.

Thanks for the update @cead22
Why aren't those tests automated?

from bedrock.

We hadn't started writing tests at the time and I didn't know how to setup the environment to run them. I asked @flodnv to get that started and now we have tests in place.

As a reminder, I wrote the original PR in May

from bedrock.

Re-created the original PR (linked above) to get this kickstarted; any reason that @tylerkaraszewski's un-revert did not have the schema changes coming with the PR anymore?

I also left them out of my PR for now; also didn't test anything yet

from bedrock.

The original PR included 2 changes: retryAfter and the schema change and when we deployed them we saw issues (commands being queued but not dequeued and processed) and we decided to split the two changes and deploy them separately.

The schema change is already deployed and now we want to deploy retryAfter to see if it causes issues

carlos@www1:~$ sudo readdb.sh ".schema jobs"
CREATE TABLE jobs ( created TIMESTAMP NOT NULL, jobID INTEGER NOT NULL PRIMARY KEY AUTOINCREMENT, state TEXT NOT NULL, name TEXT NOT NULL, nextRun TIMESTAMP NOT NULL, lastRun TIMESTAMP, repeat TEXT NOT NULL, data TEXT NOT NULL , priority INTEGER NOT NULL DEFAULT 500, parentJobID INTEGER NOT NULL DEFAULT 0 , retryAfter TEXT NOT NULL DEFAULT "");

from bedrock.

Ah, I didn't see that retryAfter is still in the schema for when creating a completely new jobs table / I didn't notice that it wasn't removed from there; so we're good 👍

from bedrock.

Can someone clarify this point

To be safe, update UpdateJob to return 402 Auto-retrying jobs cannot be updated once running if state='RUNQUEUED'. If a job is to be automatically retried, we can't have it modifying itself prior to the retry.

Why can we update the data for a RUNNING job but not a RUNQUEUED job?

cc @pecanoro

from bedrock.

Reopening as this had a performance regression and had to be rolled back.

from bedrock.