Skip to content
This repository was archived by the owner on Apr 28, 2022. It is now read-only.

Fix blocking code #191

Merged
merged 6 commits into from
Nov 1, 2020
Merged

Fix blocking code #191

merged 6 commits into from
Nov 1, 2020

Conversation

BojanPantovic1989
Copy link
Contributor

Which problem is this PR solving?

-Fixes #189

Short description of the changes

  • replace blocking collection with BufferBlock to make everything async

@BojanPantovic1989
Copy link
Contributor Author

@Falco20019 I wasn't able to test using 0.4.1, but I applied changes to make everything async.

@BojanPantovic1989 BojanPantovic1989 mentioned this pull request Sep 8, 2020
Closed
phxnsharp
phxnsharp reviewed Sep 8, 2020
View reviewed changes
phxnsharp
phxnsharp reviewed Sep 8, 2020
View reviewed changes
@phxnsharp
Copy link
Contributor

Looks great to me! Looks like maybe something went wrong the continuous integration above but I had written test cases for the original fix. If those still pass, I'm happy.

Thanks!

@Falco20019 Falco20019 added this to the v0.4.2 milestone Sep 14, 2020
@Falco20019
Copy link
Collaborator

Falco20019 commented Sep 14, 2020 •
edited
Loading

The CI failed because there were some deprecation warnings (treated as errors) from NuGET. I just fixed them. I will rerun the CI.

@BojanPantovic1989 Can you please fix the DCO? Click on the "Details" link to get instructions for it.

Falco20019
Falco20019 approved these changes Sep 14, 2020
View reviewed changes
Copy link
Collaborator

@Falco20019 Falco20019 left a comment

Choose a reason for hiding this comment

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

LGTM.

@phxnsharp Thanks for doing the review on this one! Tests are green now after fixing the pipeline.

@BojanPantovic1989
Copy link
Contributor Author

BojanPantovic1989 commented Sep 15, 2020 •
edited
Loading

Hi @Falco20019 it should be fixed now.

bojan.pantovic@redbox.com and others added 5 commits September 15, 2020 12:14
fixed blocking code in Reporters/RemoteReporter.cs
1157b20 
Signed-off-by: Bojan Pantovic <bojan.pantovic@redbox.com>
replace blocking collection with BufferBlock to make everything async
4e0e863 
Signed-off-by: Bojan Pantovic <bojan.pantovic@redbox.com>
code review changes
96b53b6 
Signed-off-by: Bojan Pantovic <bojan.pantovic@redbox.com>
code review changes jaegertracing#2
8ec1686 
Signed-off-by: Bojan Pantovic <bojan.pantovic@redbox.com>
@Falco20019
Fix NuGET related build warnings
292f4cb 
Signed-off-by: Kraemer, Benjamin <falco20019@hotmail.com>
Signed-off-by: Bojan Pantovic <bojan.pantovic@redbox.com>
@Falco20019
Copy link
Collaborator

I'm testing it a bit more right now. I found that current 0.4.1 has problems if something on the sender goes wrong. Since we use command.ExecuteAsync().Wait();, this will throw an AggregateException instead of SenderException. This PR should fix it since it's using async/await again.

@EngRajabi
Copy link

Do not publish the version ??
We have a problem with version 0.4.1

@Falco20019 Falco20019 merged commit e3b70cd into jaegertracing:master Nov 1, 2020
@Falco20019
Copy link
Collaborator

@EngRajabi Sorry, I missed that this is still open. I will release it now.

@EngRajabi
Copy link

@Falco20019 thanks

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@phxnsharp phxnsharp phxnsharp left review comments

@Falco20019 Falco20019 Falco20019 approved these changes

@yurishkuro yurishkuro Awaiting requested review from yurishkuro yurishkuro is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.4.2
Development

Successfully merging this pull request may close these issues.

RemoteReporter.cs blocking code issue
4 participants
@BojanPantovic1989 @phxnsharp @Falco20019 @EngRajabi