Skip to content

feat: improve WriteApi performance #97

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 13 commits into from
Jul 28, 2020

Conversation

bnayae
Copy link
Contributor

@bnayae bnayae commented Jun 5, 2020

Some refactoring to the WriteApi which aimed to
having more performance & simplify the code.

This is a join effort of @weknow-network & @AviAvni.

We intend to provide better simplification of the code & better performance by converting the RX channel to TPL Dataflow or System.Threading.Channels.
We will provide this changes on separate Pull Request based on this PR.
We will maintain the RX pipeline as optional strategy until you'll decide to remove it.

@bednar
Copy link
Contributor

bednar commented Jun 5, 2020

Hi @bnayae,

thanks for your PR.

Do you want to continue with #96 or close it to favour #97?

Regards

@bednar bednar added the enhancement New feature or request label Jun 5, 2020
@bnayae
Copy link
Contributor Author

bnayae commented Jun 7, 2020

this pull request depend on PointData Immutability #96.
Please check PointData Immutability #96 before this one.
I'd push fixes for #96

@bednar
Copy link
Contributor

bednar commented Jun 25, 2020

Hi @bnayae, could you please rebase the PR to avoid conflicts?

@bnayae
Copy link
Contributor Author

bnayae commented Jun 26, 2020

can you send the rebase command (and i will run it)?

Copy link
Contributor

@bednar bednar left a comment

Choose a reason for hiding this comment

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

Thanks for your work. There is only a little bit a work and we will be ready to merge it.

@codecov-commenter
Copy link

codecov-commenter commented Jul 5, 2020

Codecov Report

Merging #97 into master will decrease coverage by 0.02%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #97      +/-   ##
==========================================
- Coverage   15.67%   15.65%   -0.03%     
==========================================
  Files          50       50              
  Lines        4989     4996       +7     
  Branches      252      252              
==========================================
  Hits          782      782              
- Misses       4187     4194       +7     
  Partials       20       20              
Impacted Files Coverage Δ
Client/OrganizationsApi.cs 0.00% <0.00%> (ø)
Client/WriteApi.cs 0.00% <0.00%> (ø)
Client/Writes/PointData.cs 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d5c0e54...121301e. Read the comment docs.

Comment on lines 142 to 144
public async Task<List<Organization>> FindOrganizationsAsync()
public Task<List<Organization>> FindOrganizationsAsync()
{
return await _service.GetOrgsAsync().ContinueWith(t => t.Result.Orgs);
return _service.GetOrgsAsync().ContinueWith(t => t.Result.Orgs);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are async and await removed?

public async Task<List<Organization>> FindOrganizationsAsync()
{
    return await _service.GetOrgsAsync().ContinueWith(t => t.Result.Orgs);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought you asked for it
image

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for late. We've fixed the useless ContinueWith in #108. Could you rebase sources with master branch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't sure how to rebase it, i already push it to the fork.
I can merge and remove this issue

Copy link
Contributor

@bednar bednar left a comment

Choose a reason for hiding this comment

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

LGTM

@bnayae thanks for awesome improvements!

@bednar bednar changed the title Refactor write api feat: improve WriteApi performance Jul 28, 2020
@bednar bednar merged commit d33c8a6 into influxdata:master Jul 28, 2020
@bednar bednar added this to the 1.11.0 milestone Jul 28, 2020
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants