slicer endpoint should not hang on new slicer creation resolves #659 … #667
Conversation
lib/cluster/services/api.js
Outdated
| } | ||
| const errMsg = `could not get slicer statistics, error: ${parseError(errObj)}`; | ||
| logger.error(errMsg); | ||
| sendError(res, 500, errMsg); |
There was a problem hiding this comment.
Come on ... this code sequence is repeated 4 times... I can't explain how frustrating that is after all the time we spent trying to clean this module up.
Does this really require special casing like this? Where does that .code attribute come from? Why can't it simply be handled in the general error handler?
| @@ -601,22 +601,20 @@ module.exports = function module(context, messaging, executionService) { | |||
| })); | |||
| } | |||
|
|
|||
| function getSlicerStats(exId) { | |||
| function getSlicerStats(exIds, specificId) { | |||
There was a problem hiding this comment.
This looks questionable to me. What required this API to change in this way?
|
I am not really seeing #659 as being resolved. When I run the integration tests, I have typically been able to run Pretty easy to reproduce ... just start up the integration tests and walk through the endpoints ... They will all return except slicers. You might catch the tests between jobs and |
|
I talked to @jsnoble about this and he thinks the issue with hitting it while the integration tests are running is down to the rapid fire nature of how jobs are scheduled in the tests and tstop catching them after a job has ended but before the slicer has fully shutdown. This is a different issue and he will open a separate ticket and put a fix in separate PR. As for #659 this PR does appear to resolve that issue. The integration tests pass otherwise so going ahead with a merge. |
…#589