HPCC-13030 Add row tracing support to engines #7862
Conversation
|
https://track.hpccsystems.com/browse/HPCC-13030 |
|
@richardkchapman @jakesmith Not complete but would appreciate interim comments. Thanks. |
|
Automated Smoketest Build: success |
1 similar comment
|
Automated Smoketest Build: success |
| @@ -250,6 +250,7 @@ class InputCounter : public CSimpleInterface, implements IThorDataLink | |||
| virtual CActivityBase *queryFromActivity(); | |||
| virtual void dataLinkSerialize(MemoryBuffer &mb); | |||
| unsigned __int64 queryTotalCycles() const; | |||
| virtual void debugRequest(MemoryBuffer &msg); | |||
There was a problem hiding this comment.
minor: would be good to make the other IThorDataLink interface definitions here, declared with virtual as well at this juncture.
There was a problem hiding this comment.
Consistency.
All are virtuals and declared virtuals here, apart from queryTotalCycles(),.
|
@shamser - looks good, a few comments, mostly minor/trivial. |
|
Automated Smoketest Build: success |
| else if (strncmp(command,"quit", 4) == 0) | ||
| { | ||
| LOG(MCwarning, thorJob, "ABORT detected from user"); | ||
| Owned <IException> e = MakeThorException(TE_WorkUnitAborting, "User signalled abort"); |
There was a problem hiding this comment.
quibble: don't normally have space between Owned and <
There was a problem hiding this comment.
Note, should I also modify the original code where there is a space there also? (thgraphmaster.cpp line 1637)
|
Automated Smoketest Build: success |
|
Automated Smoketest Build: success |
|
@jakesmith Modified as per code review comments. |
|
|
||
| const __int64 diffUSec = cycle_to_microsec(get_cycles_now() - cycleStamp); | ||
| const __int64 adjustSecs = diffUSec / 1000000; | ||
| const __int64 adjustUSecs = (diffUSec % 1000000 ); |
There was a problem hiding this comment.
trivial: extra space before )
|
@shamser - final comment I think, will also nee rebasing. |
|
@jakesmith @richardkchapman Commit re-based. Over to you. |
|
Automated Smoketest Build: success |
|
@shamser - Looks good, but needs rebasing onto master |
Implement row tracing for Thor
|
@jakesmith @richardkchapman Rebased to master |
|
Automated Smoketest Build: success |
| sock->cancel_accept(); | ||
| threaded.join(); | ||
| } | ||
| virtual unsigned getPort() { return port; } |
|
@richardkchapman - ready to merge |
CDelayedInput object pointer stored in multiple locations - usage must be tracked
|
@jakesmith bug fix: was storing CDelayedInput in new array but not tracking usage. |
Make .getPort() const
|
Automated Smoketest Build: failed |
|
Automated Smoketest Build: success |
| if (queryJob().getOptBool("TRACEROWS")) | ||
| { | ||
| const unsigned numTraceRows = queryJob().getOptInt("numTraceRows", 10); | ||
| outputs.append(new CTracingThorDataLink(itdl, queryHelper(), numTraceRows)); |
There was a problem hiding this comment.
Will this work if an output ever gets cast (or query interfaced) back to its original base class?
What I am seeing is that the metaInfo (counts and recordSize etc.) are not getting reported within the graphs and suspect that something is attempting to cast *CTracingThorDataLink to *CThorDataLink (via queryOutput?) and if successful pulls the metaInfo from it?
There was a problem hiding this comment.
No, if outputs is cast back to the original base class, it won't work. We had to make changes to NSplitterSlaveActivity because of this issue. Let me see everywhere that outputs and, if necessary, modify to work with CTracingThorDataLink.
There was a problem hiding this comment.
I don't think there is (or was) any casting to the implemented class, except NSplitter's special case where it was casting to it's own derivative of CThorDataLink, which as @shamser says has been handled differently now (not using queryOutput()).
If there were any miscasting, you'd likely get a crash than missing meta info.
@GordonSmith, is the missing meta info involving particular activities/edges ?
There was a problem hiding this comment.
@GordonSmith The missing meta information is occuring in the master branch. All work-units (even one produced by prior builds) has missing metat information. If I revert to the
candidate-5.4.6, the meta information is visible again. A problem with eclwatch? Jira ticket: https://track.hpccsystems.com/browse/HPCC-14600
|
Automated Smoketest Build: success |
|
Merged manually to 6.0.0, squashed to single commit |
Return last 10 rows for a given activity & edge, when xml submitted to port 9877 (or as configured in global) in the form: