Skip to content
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

feat: add functionality to manual flush events #122

Merged
merged 3 commits into from
May 23, 2024
Merged

feat: add functionality to manual flush events #122

merged 3 commits into from
May 23, 2024

Conversation

nicklasl
Copy link
Member

@nicklasl nicklasl commented May 23, 2024

We want to allow for the user to manually trigger an upload to the backend services for all events emitted up to that point.

The approach used in this PR is to treat a call to the flush() method as a special event emitted, which will mean that the events in the "write channel" will be guaranteed to have been written to disk (and thus applicable to be part of the "batch" to upload) before the actual flush happens.

@@ -106,6 +109,10 @@ final class EventSenderEngineImpl: EventSenderEngine {
)
}

func flush() {
writeReqChannel.send(manualFlushEvent)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be an alternative?

        self.uploadReqChannel.send(EventSenderEngineImpl.sendSignalName)
        self.flushPolicies.forEach { policy in policy.reset() }

So we don't need to treat the manual flush as a policy

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I wanted to achieve with treating the flush as an event is that I want to allow all events in the write channel to be written and added to the flushed batch.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the PR description with some explanation about this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, good thinking there 👍

Copy link
Member

@fabriziodemaria fabriziodemaria left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to change the write queue to accept an internal event type (that can include the actual event), so we can model the flush event as an internal signal rather than a Confidence event. But I wouldn't block on this, +1

@nicklasl nicklasl merged commit 475df55 into main May 23, 2024
4 checks passed
@nicklasl nicklasl deleted the manual-flush branch May 23, 2024 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants