search

mp911de / spinach

Scalable Java Disque client
Apache License 2.0
34 stars 4 forks source link

Getjob NOHANG, WITHARGUMENTS #15

Closed macobo closed 8 years ago

macobo commented 8 years ago

Rudimentary PR that tries to implement things according to the feedback in #14.

Documentation still needs some work, but decided to push my work to get some preliminary feedback.

On a side note: this was surprisingly hard to write due to the repetitive interfaces - was some form of code gen used for them?

mp911de commented 8 years ago

Thx @macobo that looks pretty decent. I did not use a code generator (yet) for the interfaces; they are still manually written. For lettuce 4.0 I used a custom generator because it has a high number of methods and interfaces. That's probably a task for the future. Great that you found all the interfaces.

There is one issue I see, the GetJobArgs carries the count, which is OK in general. The point is that it's used for the single getjob method and multiple jobs use a different data structure that can run into protocol errors. To solve that situation we could either have a GetJobArgs and GetJobsArgs but to be honest: Effort is not worth the benefit.

What do you think about renaming GetJobArgs to GetJobsArgs and use it only on the getjobs method?

macobo commented 8 years ago

There is one issue I see, the GetJobArgs carries the count, which is OK in general. The point is that it's used for the single getjob method and multiple jobs use a different data structure that can run into protocol errors. To solve that situation we could either have a GetJobArgs and GetJobsArgs but to be honest: Effort is not worth the benefit. What do you think about renaming GetJobArgs to GetJobsArgs and use it only on the getjobs method?

I think we should have the singular getjob take these args - e.g. when I take a single job I might want to specify WITHCOUNTERS or NOHANG.

I tried to account for this when writing the PR by setting counter to 1 by force in the getjob command builder. This isn't the cleanest way but I think its less bad as throwing an error or duplicating the class.

Does that solution wfy or should we try to approach this by a different way?

PS: thank you for the code review - I'll bump after a response about the above!

mp911de commented 8 years ago

Ok, then proceed with the single job command with GetJobArgs.

Considering the "clean" aspect, I tend to move count back to the method args level. This way we keep getjob clean and as far as I can see the count arg affects only one getjobs method on the three interfaces (The method signature would be then getjobs(long count, GetJobArgs getJobArgs, K... queues) or getjobs(GetJobArgs getJobArgs, long count, K... queues) instead of getjobs(GetJobArgs getJobArgs, K... queues)).

macobo commented 8 years ago

Yeah, that would definitely work. I'll bump the PR later today.

macobo commented 8 years ago

I've updated the branch in accordance to the feedback! Thanks for being so responsive on a weekend.

mp911de commented 8 years ago

Thank you for your awesome contribution. Working on open source projects usually works better for me on weekends. lettuce and spinach are not part of my day-to-day job. I'd ask you just to update the docs on GetJobArgs and I'll take care of the rest.

macobo commented 8 years ago

Done. :+1:

mp911de commented 8 years ago

Merged. Thank you very much.

mp911de commented 8 years ago

I added some docs afterwards.