seomoz / qless-core

Core Lua Scripts for qless
MIT License
85 stars 33 forks source link

Fixed various minor bugs #16

Closed TriangleIL closed 11 years ago

TriangleIL commented 11 years ago

Depends.lua - Added better support for dependent jobs, missing now argument to support this case. This code should be reviewed to be sure the intent is still correct. Peek.lua - Fixed assertions with incorrect argument mappings Priority.lua - If queue was not found, incorrectly casting a boolean queue value and appending it with '' caused exception. Recur.lua - Wrapped the null JSON object in single quotes to properly expose assertion message to client. Tag.lua - "Get" changed the count to accurately return the number of items requested. "Top" modified to return the entire set of items, up to 'count' number of items.

dlecocq commented 11 years ago

Hi @TriangleIL -- sorry for not getting back to you sooner! I'm in the middle of a lot of refactoring, and so I'll probably merge this into a topic branch and rebase my refactoring off of it so that your commit can stay in the history and you can get credit for your help :-)

TriangleIL commented 11 years ago

@dlecocq Sounds great, thanks for the library. Works great for us so far! We have begun work on java bindings for qless also, they are on github as well (but are a work in progress).

dlecocq commented 11 years ago

It's uncanny how many issues that we were working on that you touched on :-) That said, I think that most of these issues have now been resolved in master. The only exception is depends, where I'm having trouble determining what the intention was -- is the hope to be able to add dependencies to jobs that are waiting, but disallow removing dependencies from jobs that aren't in state='depends'?

TriangleIL commented 11 years ago

I wasn't exactly clear on the intent for depends either, I was trying to make sure that 'on' reflected the inverse of 'off'. In the off case, we were removing from the -depends queue for the passed in jobs, I thought we wanted to add to the depends queue in the 'on' case. Perhaps my understanding of the depends script wasn't clear, that is why I suggested it would need review. Thanks again for the qless library, we are looking to update the qless java bindings to the unified core soon.

dlecocq commented 11 years ago

Yeah, historically, we haven't supported getting a job into the 'depends' state if it's not already in the 'depends' state, in which case it's already in the 'depends' queue. Mostly that was to avoid having to deal with issues about what if the job is running or scheduled, though with 'unified' we're in more of a position to deal with those issues.

That said, an alternative to putting a job into the depends state is to re-put the job with the same parameters, with the addition of the new dependencies.

I'm glad you're finding some use out of it! Another new updated to qless-core moves all of the unit tests out of the bindings and into this repo, so now clients are responsible for far fewer tests, all of which are focused on integration. We've not yet made the testing update to the ruby bindings, but for an example, you can see the python bindings for an example of the integration tests that you need (if it helps).

We've also talked about getting a qless org together on github so that all the bindings in all the languages could live together, so let me know how the java bindings are going.