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

Cache version per influxdb instance and reduce ping() calls for every… #472

Merged
merged 2 commits into from
Jul 21, 2018

Conversation

majst01
Copy link
Collaborator

@majst01 majst01 commented Jul 21, 2018

… query call, closes #470

@majst01 majst01 requested a review from fmachado July 21, 2018 09:57
@codecov-io
Copy link

codecov-io commented Jul 21, 2018

Codecov Report

Merging #472 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #472      +/-   ##
============================================
+ Coverage     86.63%   86.65%   +0.02%     
- Complexity      302      303       +1     
============================================
  Files            20       20              
  Lines          1294     1296       +2     
  Branches        135      135              
============================================
+ Hits           1121     1123       +2     
  Misses          113      113              
  Partials         60       60
Impacted Files Coverage Δ Complexity Δ
src/main/java/org/influxdb/impl/InfluxDBImpl.java 87.76% <100%> (+0.07%) 69 <2> (+1) ⬆️

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 f8bd921...96580fe. Read the comment docs.

@@ -313,7 +314,10 @@ public Pong ping() {

@Override
public String version() {
return ping().getVersion();
if (version == "") {
Copy link
Contributor

@fmachado fmachado Jul 21, 2018

Choose a reason for hiding this comment

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

You are still programming in Javascript here. :-P
What about replacing "" with null here and remove "" from the variable declaration?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are right, to much different languages, java is getting out of my head by the time :-). Fixed

@majst01 majst01 merged commit c57c09d into master Jul 21, 2018
@majst01 majst01 deleted the cache-version branch July 21, 2018 11:01
@lxhoan
Copy link
Contributor

lxhoan commented Jul 23, 2018

I would like to ask how the client can know if the server side is upgraded to a new version ?

@majst01
Copy link
Collaborator Author

majst01 commented Jul 23, 2018

The client cant, we promise compatibility to 1.1 onwards. We could probably add a check in the constructor which crashes once, but the current code does this check on every query which is awkward.

What do you think ?

@lxhoan
Copy link
Contributor

lxhoan commented Jul 23, 2018

A check (for server version) in the constructor is also the approach I would like to use for MessagePack so of course I would vote for it.

For the cached version, it may go a bit confused and inconsistent (when the server side version changes, yet not very likely) for user so I just think we can document the method version() explicitly on this

@lxhoan lxhoan added this to the 2.12 milestone Jul 27, 2018
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.

Please remove ping checks from query method
4 participants