-
Notifications
You must be signed in to change notification settings - Fork 47
Historyserver #10
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
Historyserver #10
Conversation
| numOutputRows += task->getNumOutputRows(); | ||
| } | ||
| } else { | ||
| auto rs = tstage->resultSet(); |
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.
does this "consume" the resultset?
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.
Not exactly sure what you mean by "consume".
| // delete task | ||
| delete tasks[i]; | ||
| tasks[i] = nullptr; | ||
| // delete tasks[i]; |
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.
why is this commented? bug?
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 is commented because it was leading to a bug. This delete was added during the open sourcing, as it was not on my old branch in the old repo. The bug was the following: executeTransformStage calls createFinalHashmap. createFinalHashmap ends up deleting the tasks. Later in executeTransformStage, if the webUI is enabled, we have to iterate through the tasks to get the number of output rows. However, this was causing a nullptr dereference. So I commented the delete tasks[I] and setting it to a nullptr. If the memory is not being managed elsewhere, the tasks can be deleted after the history server is done using them and then deleted in an else statement if webUI not enabled.
|
Note: Before merging to master, I would like to write some of my own comprehensive tests to ensure everything is working. Basically using the history server to look at complex pipelines with many error rows. |
LeonhardFS
left a comment
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.
can you go over the comments? we probably should also do a functional test and check that the HTML/JS works well.
Great work so far getting this back working! Excited to have it shipped soon in future release 0.3.3.
|
superseded by #38 |
This is added support for the WebUI. It supports previous functionality of a much older version of Tuplex as well as new support for splitting the plan into stages, tracking stage dependencies, join operators, aggregate operators, and showing UDF's for filter operators.
It would be great if you could do a code review and tell me what I should change.