-
Notifications
You must be signed in to change notification settings - Fork 104
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add order parameter to jobs inspecting API #37
Conversation
jobqueue/mysql/inspector.go
Outdated
} | ||
} | ||
|
||
func (i *inspector) findAllAsc(status string, minTime int64, maxTime int64, limit uint, cursor string) (*jobqueue.InspectedJobs, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method has many duplicated code with findAllDesc
🤔 but I cannot split these method as simple.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't surprise me. There is duplicated code already in failureLog
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Sixeight It looks almost OK. I have some minor change requests.
jobqueue/mysql/inspector.go
Outdated
maxTime = int64(t) | ||
maxJobID = uint64(j) | ||
*time = int64(t) | ||
*jobID = uint64(j) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This mutation looks tricky to me. Consider making this function immutable by taking default values and returning a pair:
func decodeCursor(cursor string, defaultTime int64, defaultJobID uint64) (time int64, jobID uint64)
jobqueue/mysql/inspector.go
Outdated
} | ||
} | ||
|
||
func (i *inspector) findAllAsc(status string, minTime int64, maxTime int64, limit uint, cursor string) (*jobqueue.InspectedJobs, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't surprise me. There is duplicated code already in failureLog
.
if len(r2.Jobs) != 0 { | ||
t.Error("There must be no waiting job in the queue") | ||
} | ||
|
||
r3, err := i.FindAllDeferred(uint(100), "") | ||
r3, err := i.FindAllDeferred(uint(100), "", jobqueue.Desc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be better to have tests for asceding order too.
@tarao Thank you for your review. I've corresponded to your point. Could you review again? |
ecb85d9
to
3e86a12
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Sixeight LGTM
Thank you! |
What
order
parameter for jobs inspecting API. We can inspect jobs in arbitrary order byfnext_try
field.Why
/queue/default/waiting?order=asc&limit=1
satisfies the behavior.