Skip to content
This repository has been archived by the owner on Aug 30, 2023. It is now read-only.

fix: send cached events to use a single thread executor #405

Merged
merged 22 commits into from
May 16, 2020
Merged

fix: send cached events to use a single thread executor #405

merged 22 commits into from
May 16, 2020

Conversation

marandaneto
Copy link
Contributor

@marandaneto marandaneto commented May 13, 2020 •
edited
Loading

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

send cached events to use a single executor.

💡 Motivation and Context

💚 How did you test it?

running it

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • All tests passing

🔮 Next steps

@marandaneto marandaneto requested a review from bruno-garcia May 13, 2020 08:10
@bruno-garcia
Copy link
Member

We discussed this offline but I wanted to leave one note: Let please leave some sort of timeout. Even if it's 30 seconds. IMHO 30 seconds waiting for a single event to be flushed is a lot but let's just not let the thread block forever without timeout at least

@marandaneto marandaneto changed the title cached events should not have timeout send cached events use a single executor May 14, 2020
@marandaneto marandaneto changed the title send cached events use a single executor send cached events to use a single executor May 14, 2020
bruno-garcia
bruno-garcia suggested changes May 15, 2020
View reviewed changes
Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

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

There are some core bits changing here. I left some notes. Let me know if you'd like to discuss this on a call.

sentry-core/src/main/java/io/sentry/core/Hub.java Outdated Show resolved Hide resolved
sentry-sample/src/main/AndroidManifest.xml Outdated Show resolved Hide resolved
sentry-core/src/main/java/io/sentry/core/EnvelopeSender.java Outdated Show resolved Hide resolved
sentry-core/src/main/java/io/sentry/core/SendCachedEvent.java Outdated Show resolved Hide resolved
sentry-core/src/main/java/io/sentry/core/SendCachedEvent.java Outdated Show resolved Hide resolved
sentry-core/src/main/java/io/sentry/core/SendCachedEvent.java Outdated Show resolved Hide resolved
sentry-core/src/main/java/io/sentry/core/SendFireAndForgetEnvelopeSender.java Outdated Show resolved Hide resolved
sentry-core/src/main/java/io/sentry/core/SendCachedEventFireAndForgetIntegration.java Show resolved Hide resolved
sentry-core/src/main/java/io/sentry/core/SentryOptions.java Outdated Show resolved Hide resolved
sentry-core/src/main/java/io/sentry/core/transport/HttpTransport.java Outdated Show resolved Hide resolved
@marandaneto marandaneto marked this pull request as ready for review May 15, 2020 14:53
@marandaneto marandaneto requested a review from bruno-garcia May 15, 2020 14:59
bruno-garcia
bruno-garcia approved these changes May 15, 2020
View reviewed changes
Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

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

👍 nice let's see how it goes

sentry-core/src/test/java/io/sentry/core/SentryClientTest.kt Outdated Show resolved Hide resolved
@marandaneto marandaneto changed the title send cached events to use a single executor fix: send cached events to use a single thread executor May 15, 2020
@marandaneto marandaneto merged commit 8b964c6 into getsentry:master May 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bruno-garcia bruno-garcia bruno-garcia approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@marandaneto @bruno-garcia