search

seomoz / qless-core

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

Added support for max-job-history #26

Closed dlecocq closed 11 years ago

dlecocq commented 11 years ago

For #25

myronmarston commented 11 years ago

That was quick -- thanks! :).

I mentioned this in #25, but I think it makes sense to always keep the very first event in the history (e.g. the put that allowed the job to enter qless for the first time). There's a lot of value to that first event. For your test, it would have the put events at these times: [0, 96, 97, 98, 99].

Thoughts?

dlecocq commented 11 years ago

Ah, I apparently glossed over that when I read the issue. I'm gonna ponder that a bit -- it's hard to know where the cutoff should be. Like, should we keep the first 1 event, or the first 2, 3, k?

I'd be potentially on board with something representative of both sides, but just not sure what yet. On the other hand, I could see a use-case where we just want to keep the most recent history. On the other hand, one more event (the first one) almost certainly couldn't hurt.

myronmarston commented 11 years ago

Ah, I apparently glossed over that when I read the issue. I'm gonna ponder that a bit -- it's hard to know where the cutoff should be. Like, should we keep the first 1 event, or the first 2, 3, k?

I think the first event is orders-of-magnitude more important than the others. Without it, you have no way to tell how long the job has been banging around in the system. With it, you have a trivial way to tell that the job is actually 6 weeks old (or whatever). For the 2nd, 3rd and Kth events, I care far less; there aren't any fundamental questions (I can think of, anyway) that I might be asking that those events are needed to answer.

dlecocq commented 11 years ago

Now it's keeping the first history item around, and defaults to 100 entries.

myronmarston commented 11 years ago

LGTM. Thanks, Dan!