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

prepare code changes to support UDP async #239

Merged
merged 2 commits into from
Nov 12, 2016

Conversation

jiafu1115
Copy link
Contributor

@jiafu1115 jiafu1115 commented Nov 10, 2016

This is another PR to prepare supporting UDP async.
Create this PR's goal to make sure we can step by step with limit code changes for clear review.

abstract batch entry to support both http batch entry and udp batch entry.

This will avoid use the same batch entry due to http batch entry doesn't need udp port and udp patch doesn't need dbname or rp.

I doesn't add any unit tests(so decrease coverage by 0.42%) due to just refactor and no use the UdpBatchEntry just now. it is just one common java bean without any logical.
I will add tests together with the next PR when support UDP patch officially.

@jiafu1115 jiafu1115 changed the title refactor code for UDP async batch support prepare code changes to support UDP async batch Nov 10, 2016
@codecov-io
Copy link

codecov-io commented Nov 10, 2016

Current coverage is 68.49% (diff: 68.18%)

Merging #239 into master will decrease coverage by 0.08%

@@             master       #239   diff @@
==========================================
  Files            10         10          
  Lines           576        584     +8   
  Methods           0          0          
  Messages          0          0          
  Branches         68         69     +1   
==========================================
+ Hits            395        400     +5   
- Misses          147        150     +3   
  Partials         34         34          

Powered by Codecov. Last update 739929d...6355973

@majst01
Copy link
Collaborator

majst01 commented Nov 10, 2016

Build fails !

@majst01
Copy link
Collaborator

majst01 commented Nov 10, 2016

No one unit-test is failing:
see https://travis-ci.org/influxdata/influxdb-java/builds/174660265#L4749

@jiafu1115
Copy link
Contributor Author

jiafu1115 commented Nov 10, 2016

@majst01 build fail is caused by sometime UDP's write executed on server is after http's query. so I may sleep 2s to avoid the fail.
note: http is slow udp. but we can't make sure the first request with udp is faster than second request with http. So I have to sleep 1-2 s to make sure it. Or I may need try with times.

@jiafu1115
Copy link
Contributor Author

@majst01 It is ok now. You can go ahead with your review. If the similar build error happen again, I may change the design for the test cases to avoid sleeping 2s but just try times with interval.

@majst01
Copy link
Collaborator

majst01 commented Nov 10, 2016

I'm still not sure why we need batchprocessing for udp because udp is fire and forget anyway.

Can you please provide some metrics which show the gains of using batchprocessing for udp.
This results should also be added to some documentation.

@jiafu1115
Copy link
Contributor Author

jiafu1115 commented Nov 10, 2016

@majst01
batch is to improve performance due to reduce send times and make more easier to use.
async mode is to avoid influence business thread.

There are three points for batch processing goal

(1) improve performance with batch
when we have three points, if one by one, we should create three udp package and sent 3 times.
if support batch, we can create one package and send 1 time.

(2) to avoid pass same tags into every points. this point is no relationships for performance. It will make api users more easier to use it:

such as : if we not support batch, we may write code when existed many same tags as followed:

point one = point.tag("sametagkey1","sametagkey1").tag("sametagkey2","sametagkey2").field("1","2").build
point two = point.tag("sametagkey1","sametagkey1").tag("sametagkey2","sametagkey2").field("3","4").build

influxdb.write(udpport, Arrays.asList(one, two))

the if we support batch:

UdpPatch udpPatch =UdpPatch.tag("sametagkey1","sametagkey1").tag("sametagkey2","sametagkey2").build;
point one = point.field("1","2").build
point two = point.field("3","4").build

influxdb.write(udpport, udpPatch )

(3) http support it. we should keep udp support too.

What's more, this PR take more focuses on async support. not just limit to batch. maybe I need to change title. And the next PR is to support async UDP with little code changes and more test cases.. Then we can consider how to support UdpBatches instead of List

BTW: When we complete all UDP‘s support, I will provide one compare with http with udp through as everyone know udp is must be faster than http.

Thanks again.

WDYT?

@jiafu1115 jiafu1115 changed the title prepare code changes to support UDP async batch prepare code changes to support UDP async and batch Nov 10, 2016
@jiafu1115 jiafu1115 changed the title prepare code changes to support UDP async and batch prepare code changes to support UDP async Nov 10, 2016
@jiafu1115
Copy link
Contributor Author

jiafu1115 commented Nov 11, 2016

@majst01 Can you continue this PR now so that I can complete support UDP async today with little code changes. I pay more attention of this PR's reason is that I can't go ahead to fix other issues to resolve possible code conflict with other fixs' commits. So can you also take more time to pay more attention on this PR when you are free.

Then we would completed all big features such as UDP and Gzip. Then we can take a rest!

@majst01 majst01 merged commit 359a9b6 into influxdata:master Nov 12, 2016
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.

3 participants