Formed in 2009, the Archive Team (not to be confused with the archive.org Archive-It Team) is a rogue archivist collective dedicated to saving copies of rapidly dying or deleted websites for the sake of history and digital heritage. The group is 100% composed of volunteers and interested parties, and has expanded into a large amount of related projects for saving online and digital history.
Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upMINIFICPP-1349 ConsumeWindowsEventLogs should honor batch commit size in a single session #891
Conversation
| } | ||
|
|
||
| std::wstring ConsumeWindowsEventLog::processEventLogs(const std::shared_ptr<core::ProcessContext> &context, const std::shared_ptr<core::ProcessSession> &session, | ||
| size_t& processed_event_count, const EVT_HANDLE& event_query_results) { |
This comment has been minimized.
This comment has been minimized.
fgerlits
Sep 3, 2020
Contributor
I would do this the other way round: return the number of events processed, and use an out parameter for the bookmark XML. Not important, but I think that is a more common pattern.
Another option would be to return a struct { size_t processed_event_count; std::wstring bookmark_xml; }.
This comment has been minimized.
This comment has been minimized.
lordgamez
Sep 8, 2020
Author
Contributor
As both are output parameters I decided to return a tuple with both values. Added in 24855ee
|
|
||
| if (eventCount > commitAndSaveBookmarkCount) { | ||
| commitAndSaveBookmark(bookmarkXml); | ||
| if (processed_event_count > 0 && !commitAndSaveBookmark(bookmark_xml, session)) { |
This comment has been minimized.
This comment has been minimized.
fgerlits
Sep 3, 2020
Contributor
I think we should yield if no events were processed, too. I know we didn't before this change, but that seems like a bug.
| if (processed_event_count > 0 && !commitAndSaveBookmark(bookmark_xml, session)) { | |
| if (processed_event_count == 0 || !commitAndSaveBookmark(bookmark_xml, session)) { |
This comment has been minimized.
This comment has been minimized.
| } | ||
|
|
||
| std::wstring ConsumeWindowsEventLog::processEventLogs(const std::shared_ptr<core::ProcessContext> &context, const std::shared_ptr<core::ProcessSession> &session, | ||
| size_t& processed_event_count, const EVT_HANDLE& event_query_results) { |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
lordgamez
Sep 8, 2020
Author
Contributor
Unfortunately no, because of getEventLogHandler in the createEventRender call. But I made 2 other methods const in 0a32d4e
| batchCommitSizeTestHelper(1000); | ||
| batchCommitSizeTestHelper(5); | ||
| batchCommitSizeTestHelper(4); | ||
| batchCommitSizeTestHelper(1); | ||
| batchCommitSizeTestHelper(0); |
This comment has been minimized.
This comment has been minimized.
fgerlits
Sep 3, 2020
Contributor
I think the test is clearer if you explicitly specify the expected result, as before. Maybe the number of events could be a parameter, too, eg. batchCommitSizeTestHelper(int num_events_read, int batch_size, int num_events_processed).
| batchCommitSizeTestHelper(1000); | |
| batchCommitSizeTestHelper(5); | |
| batchCommitSizeTestHelper(4); | |
| batchCommitSizeTestHelper(1); | |
| batchCommitSizeTestHelper(0); | |
| batchCommitSizeTestHelper(5, 1000, 5); | |
| batchCommitSizeTestHelper(5, 5, 5); | |
| batchCommitSizeTestHelper(5, 4, 4); | |
| batchCommitSizeTestHelper(5, 1, 1); | |
| batchCommitSizeTestHelper(5, 0, 5); |
This comment has been minimized.
This comment has been minimized.
|
|
||
| REQUIRE(LogTestController::getInstance().countOccurrences("processQueue commit") == expected_num_commits); | ||
| } | ||
| std::vector<std::string> events{"Event one", "Event two", "Event three", "Event four", "Event five"}; |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
lordgamez
Sep 8, 2020
Author
Contributor
Maybe I just wanted to keep it in a single line, but yea I see it's better to have a simple for loop here, fixed in fa06429
lordgamez commentedSep 2, 2020
•
edited
Thank you for submitting a contribution to Apache NiFi - MiNiFi C++.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
For all changes:
Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
Does your PR title start with MINIFICPP-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
Has your PR been rebased against the latest commit within the target branch (typically main)?
Is your initial contribution a single, squashed commit?
For code changes:
For documentation related changes:
Note:
Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.