Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Configurable timeout #8

Merged
merged 7 commits into from

2 participants

@lightsofapollo

This includes a json schema change and a database schema change (both break stuff)... I also found what appeared to be a bug in how retries where decremented beyond 0.

@jonasfj
Owner

Why not just test if $AWS_ACCESS_KEY_ID and $AWS_SECRET_ACCESS_KEY are defined?

Some of us have them defined locally.. and wouldn't mind running them too...

@jonasfj

Could we add:

"The queue will take the `timeout` value as an advice, but reserves the right to ignore it." +
"So workers and other interested parties should **not** rely on the value of the `timeout` property."

Also, just fyi the documentation viewer renders descriptions (and titles) as markdown, enjoy :)

@jonasfj

Does this work for multiple tests?

I tried exactly that in api.js and found that I can't run it in the same process as data.js ... Nor can I restart the server for every test as the pg.js (postgres connector) refuses to reset the connection.

There is a comment in tests/index.js about using process isolation to fix this issue. I guess another solution would be to start the queue as a background process using a wrapper like LocalWorker as done in docker-worker.

Note, I'll look into this and probably do the wrapper thing, I'll need it for testing the post-signed-... thingy...

Owner

So, Just ignore this issue...

I had some code that worked with multiple tests in a different project... I am not 100% sure what the issue is with pg here

@jonasfj

Shouldn't this be a required property? At least in the task status structure.

This is ensured by adding it the required list of properties, on line 85.

@jonasfj

I noticed that you added timeout to all validation tests and called this PR from API breaking, do you intent timeout to be a required property?

In which case you should add it to the list of required properties, below. As you have a default value for timeout this is not necessary. So this is a question of you intent to do?

I generally haven't been a fan of optional properties, especially because it feel like something we should add when we actually want to maintain backward compatibility (which isn't a priority now). On the other hand there is little reason to submit a timeout, if the queue has a default timeout and you don't care about this.

So in this case I'm fine with either solution. Just that maybe we should have a validation test without timeout if it is intended to be optional.

@jonasfj

Of course, we're we're decrementing retries on task claim... Nice catch...

@lightsofapollo lightsofapollo merged commit 7671620 into from
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.