HPCC-33665: Define a function to open and parse binary event data files by timothyklemm · Pull Request #19658 · hpcc-systems/HPCC-Platform

timothyklemm · 2025-03-25T15:07:49Z

Declare the IEventVisitor interface to receive parts of a binary event data file as they are parsed.
Define JLib method readEvents to open and parse one event data file. Each datum is passed to an IEventVisitor instance in the order it appears in the file.
Define a primitive IEventVisitor implementation for illustration and testing.

Type of change:

This change is a bug fix (non-breaking change which fixes an issue).
This change is a new feature (non-breaking change which adds functionality).
This change improves the code (refactor or other change that does not change the functionality)
This change fixes warnings (the fix does not alter the functionality or the generated code)
This change is a breaking change (fix or feature that will cause existing behavior to change).
This change alters the query API (existing queries will have to be recompiled)

Checklist:

Smoketest:

Send notifications about my Pull Request position in Smoketest queue.
Test my draft Pull Request.

Testing:

github-actions · 2025-03-25T15:08:08Z

Jira Issue: https://hpccsystems.atlassian.net//browse/HPCC-33665

Jirabot Action Result:
Workflow Transition To: Merge Pending
Updated PR

- Declare the IEventVisitor interface to receive parts of a binary event data file as they are parsed. - Define JLib method readEvents to open and parse one event data file. Each datum is passed to an IEventVisitor instance in the order it appears in the file. - Define a primitive IEventVisitor implementation for illustration and testing. Signed-off-by: Tim Klemm <Tim.Klemm@lexisnexisrisk.com>

ghalliday

@timothyklemm a set of comments. I think it would be worth addressing/discussing most of them, but I am inclined to merge as-is, and then look for follow up PRs that address the comments.

ghalliday · 2025-03-26T13:03:06Z

+    size32_t got = 0;
+    if (len)
+    {
+        const char* s = (const char*)stream.peek(len, got);


minor. You should be able to use stream.read(len) instead to simplify the code.

ghalliday · 2025-03-26T13:04:25Z

+    }
+    else
+    {
+        for (;;) {


Later it would make sense for this to be extracted as a separate function.

ghalliday · 2025-03-26T13:05:32Z

+}
+
+//Abstract interface for binary event file traversal.
+interface IEventFile : extends IInterface


Something to discuss - I don't think this is needed/gives you any benefit.

ok. I see you have used it because you are thinking you may want different class implementations for different versions.
I would expect a single EventReader class where the version is pased in (as you have it already), but very unlikely to have multiple implementations - so I would simplify the calling code and assume a single reader class.

ghalliday · 2025-03-26T13:08:25Z

+        for (;;)
+        {
+            // no more data means no more events
+            size32_t got = 0;


There should be a null event terminator, so read could be used?

It would be simpler to exit the loop on EventNone if it was included. For the record, I probably would have passed it to the visitor, like I am passing EvAttrNone.

ghalliday · 2025-03-26T13:10:28Z

+            readToken(stream, attr, bytesRead);
+            if (EvAttrNone == attr)
+            {
+                if (!finishAttribute(attr, visitor, mute))


minor: I don't think a none attr should be passed through, that is an implementation detail of the file format.

I was looking at this as an implied End Of Event notification, without adding a leaveEvent (which is what the name would have been, to match leaveFile). Would you want an explicit "this event is complete" method, or would you expect visitors to terminate events in progress before starting the next or leaving the file?

I think I would expect an explicit leave function if that is passed through to the visitor.

ghalliday · 2025-03-26T16:51:19Z

+
+//Read a strongly typed value from a buffered stream.
+template<typename T>
+static T readToken(IBufferedSerialInputStream& stream, T& token, size32_t& bytesRead)


If these were member functions they would only need a single parameter - so calling code would be simpler/cleaner.

Because I prepared for multiple classes to support multiple versions, I needed to read the version token before creating the class to read everything else. If we limit it to one class for all versions, as you suggested elsewhere, I agree member methods would be cleaner.

I would recommend a single class - I can't think of any examples in the platform where we have used multiple classes for different versions like this.

ghalliday · 2025-03-26T16:57:11Z

+    if (!fileIO)
+        throw makeStringExceptionV(-1, "file '%s' not opened for reading", file.queryFilename());
+    Owned<ISerialInputStream> baseStream = createSerialInputStream(fileIO);
+    if (!baseStream)


minor: this can not return null.

ghalliday · 2025-03-26T16:57:29Z

+    if (!baseStream)
+        throw makeStringExceptionV(-1, "file '%s' input stresm not created", file.queryFilename());
+    Owned<IBufferedSerialInputStream> bufferedStream = createBufferedInputStream(baseStream, 0x100000, false);
+    if (!bufferedStream)


similarly, this can not return null

ghalliday · 2025-03-26T16:58:14Z

    EvAttrPath,
    EvAttrConnectId,
    EvAttrEnabled,
+    EvAttrSysFileSize,


naming: Avoid the Sys prefix - some attributes e.g. FileSize may well be provided for events/meta as well.

I wanted to differentiate the file being visited from all of the files referenced within that file. The prefix is also meant to distinguish auto-generated values from event data. Would you remove the prefix from all of the attributes, or just those that might be reused by an event?

Personally I would remove sys from all attributes. I might also implement the timestamp differently, but that is a separate discussion.

ghalliday · 2025-03-26T17:02:57Z

+    virtual Continuation visitAttribute(EventAttr id, uint16_t value) = 0;
+    virtual Continuation visitAttribute(EventAttr id, uint32_t value) = 0;
+    virtual Continuation visitAttribute(EventAttr id, uint64_t value) = 0;
+    virtual void leaveFile(uint32_t bytesRead) = 0;


not sure I like the name leave - ideally the verbs should be paired e.g. begin/end enter/leave.
I could also be hyper-picky about visit as a prefix, but I don't care enough!

Copilot

Copilot wasn't able to review any files in this pull request.

Files not reviewed (3)

system/jlib/jevent.cpp: Language not supported
system/jlib/jevent.hpp: Language not supported
testing/unittests/jlibtests2.cpp: Language not supported

timothyklemm

I'm OK with it merging now, with updates to follow. I'm close to having a first pass for evtool dump that depends on this, so merging will make that easier to deal with.

timothyklemm · 2025-03-26T19:07:12Z

+
+//Read a strongly typed value from a buffered stream.
+template<typename T>
+static T readToken(IBufferedSerialInputStream& stream, T& token, size32_t& bytesRead)


Because I prepared for multiple classes to support multiple versions, I needed to read the version token before creating the class to read everything else. If we limit it to one class for all versions, as you suggested elsewhere, I agree member methods would be cleaner.

timothyklemm · 2025-03-26T19:12:12Z

+        for (;;)
+        {
+            // no more data means no more events
+            size32_t got = 0;


It would be simpler to exit the loop on EventNone if it was included. For the record, I probably would have passed it to the visitor, like I am passing EvAttrNone.

timothyklemm · 2025-03-26T19:21:33Z

+            readToken(stream, attr, bytesRead);
+            if (EvAttrNone == attr)
+            {
+                if (!finishAttribute(attr, visitor, mute))


I was looking at this as an implied End Of Event notification, without adding a leaveEvent (which is what the name would have been, to match leaveFile). Would you want an explicit "this event is complete" method, or would you expect visitors to terminate events in progress before starting the next or leaving the file?

timothyklemm · 2025-03-26T19:35:12Z

    EvAttrPath,
    EvAttrConnectId,
    EvAttrEnabled,
+    EvAttrSysFileSize,


I wanted to differentiate the file being visited from all of the files referenced within that file. The prefix is also meant to distinguish auto-generated values from event data. Would you remove the prefix from all of the attributes, or just those that might be reused by an event?

timothyklemm force-pushed the hpcc-33665-event-reader branch from c0cb0c6 to 632630a Compare March 25, 2025 15:12

timothyklemm requested a review from ghalliday March 25, 2025 15:13

ghalliday approved these changes Mar 26, 2025

View reviewed changes

ghalliday requested a review from Copilot March 26, 2025 19:40

Copilot AI reviewed Mar 26, 2025

View reviewed changes

timothyklemm commented Mar 26, 2025

View reviewed changes

ghalliday merged commit b4a142c into hpcc-systems:master Mar 31, 2025
51 of 54 checks passed

Conversation

timothyklemm commented Mar 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Type of change:

Checklist:

Smoketest:

Testing:

Uh oh!

github-actions Bot commented Mar 25, 2025

Uh oh!

ghalliday left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

timothyklemm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

timothyklemm commented Mar 25, 2025 •

edited

Loading