Skip to content
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

getJobLogs pagination reversed #2031

Closed
tjhiggins opened this issue Apr 28, 2021 · 5 comments · Fixed by taskforcesh/bullmq#538
Closed

getJobLogs pagination reversed #2031

tjhiggins opened this issue Apr 28, 2021 · 5 comments · Fixed by taskforcesh/bullmq#538

Comments

@tjhiggins
Copy link
Contributor

tjhiggins commented Apr 28, 2021

Description

Implementing a UI over the bull job logs and noticed that the log pagination is reversed. I'm not sure if this is intentional, but its certainly confusing and makes implementation difficult when trying to paginate or live log stream.

Minimal, Working Test code to reproduce the issue.

This is a simple example that illustrates the problem.

job.log('Page 1');
job.log('Page 2');
job.log('Page 3');

// actual
queue.getJobLogs(job.id, 0, 0) === { count: 3, logs: ['Page 3'] }
queue.getJobLogs(job.id, 1, 1) === { count: 3, logs: ['Page 2'] }
queue.getJobLogs(job.id, 2, 2) === { count: 3, logs: ['Page 1'] }

queue.getJobLogs(job.id, 0, 1) === { count: 3, logs: ['Page 2', 'Page 3'] }

// expected
queue.getJobLogs(job.id, 0, 0) === { count: 3, logs: ['Page 1'] }
queue.getJobLogs(job.id, 1, 1) === { count: 3, logs: ['Page 2'] }
queue.getJobLogs(job.id, 2, 2) === { count: 3, logs: ['Page 3'] }

queue.getJobLogs(job.id, 0, 1) === { count: 3, logs: ['Page 1', 'Page 2'] }

Bull version

v3.22.4

@manast
Copy link
Member

manast commented Apr 28, 2021

Actually I do not remember if it was intentional or not, but yes it should not be reversed.

@manast manast added bug and removed enhancement labels Apr 28, 2021
tjhiggins pushed a commit to tjhiggins/bull that referenced this issue Apr 30, 2021
@manast
Copy link
Member

manast commented May 16, 2021

yes, I think the rationale was that often times you are interested in the newest logs and you browse backwards in time. So I think the solution to support both cases (as it works now and streaming logs), would be to add an "asc" parameter as we have in the other getters, by default would be false, and passing true will provide the functionality you are requesting.

@tjhiggins
Copy link
Contributor Author

@manast I'm fine with updating, but what do you think about this comment?
https://github.com/taskforcesh/bullmq/pull/538/files#r633169826

I'd tech be a breaking change, but if the default behavior is truly descending then we should fix the existing logic.

@manast
Copy link
Member

manast commented May 17, 2021

In my comment here taskforcesh/bullmq#538 (comment) I wrote that I think ASC should be default since current one is actually ASC (f you call the API without start, end params), so that would probably the less "breaking change", since it is not documented the expected order anyway.

@manast manast closed this as completed in 30aa0a9 May 17, 2021
github-actions bot pushed a commit that referenced this issue May 17, 2021
## [3.22.6](v3.22.5...v3.22.6) (2021-05-17)

### Bug Fixes

* **job:** fix job log pagination, fixes [#2031](#2031) ([30aa0a9](30aa0a9))
@manast
Copy link
Member

manast commented May 17, 2021

🎉 This issue has been resolved in version 3.22.6 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants