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

Add database creation/deletion for Influx 1.8 #544

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

Pitastic
Copy link

Add features related to #541 and #259

Proposed Changes

In order to maintain a minimum of compatibility to Influx v1.8 the creation an deletion of buckets (old: databases) should be supported. With these little enhancements the influxdb-client-python could easily.

I implemented two new methods for calls to v1 API if the method for creating or deleting a bucket fails with an ApiException. That way no code changes are necessary regardless which version of Influx you are running on.

I implemented this with a call to the method influxdb_client.api_client.call_api instead of using the _buckets_service.post_buckets as this was easier and with less code changes. I had to invoke the creation of some arguments and hope you find it clean enough.

Checklist

As I just changed functionallity which wasn't there before and doesn't effect any of the other methods I don't know what to test or how to write a test for this. If this or anything else is needed, please advice me to the right direction.

@telegraf-tiger
Copy link

Thanks so much for the pull request!
🤝 ✒️ Just a reminder that the CLA has not yet been signed, and we'll need it before merging. Please sign the CLA when you get a chance, then post a comment here saying !signed-cla

@Pitastic
Copy link
Author

!signed-cla

@Pitastic
Copy link
Author

The fails left are related to a numpy problem.

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 PR 👍

Please rebase your sources to fix CI build - #543

Comment on lines 58 to 62
try:
return self._buckets_service.post_buckets(post_bucket_request=bucket)
except ApiException:
# Fall back to v1 API if buckets are not supported
database_name = bucket_name if bucket_name is not None else bucket
Copy link
Contributor

Choose a reason for hiding this comment

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

This approach is too general and can lead to user confusion. The better way would be to create a separate API for InfluxDB 1.8 something like influxdb_18_api.py.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for reviewing.

I would disagree to create a separate API (If I understand your suggestion right). The goal should be that code changes are not necessary regardless of what version of InfluxDB a user uses (1.8 or 2.0 and above).

create_bucket() should always succeed for both versions without extra error handling. Otherwise users could continue using the old influxdb-client for 1.8 and this one for >2.0 .

My suggestion would be to implement a more specific exception handler. I agree that just ApiException is too general. I will have a look into the returned ApiException.code.

Would this be acceptable for your project?

Copy link
Contributor

Choose a reason for hiding this comment

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

We can accept this if the behaviour will be described in the documentation. Instead of catching exception you can check the InfluxDB version, something like here:

def _is_cloud_instance(self) -> bool:

@Pitastic
Copy link
Author

Thanks for your PR 👍

Please rebase your sources to fix CI build - #543

Will do 👍

@Pitastic Pitastic marked this pull request as draft December 28, 2022 09:27
@codecov-commenter
Copy link

codecov-commenter commented Dec 28, 2022

Codecov Report

Base: 90.36% // Head: 89.72% // Decreases project coverage by -0.64% ⚠️

Coverage data is based on head (83deff4) compared to base (03f5bd7).
Patch coverage: 13.79% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #544      +/-   ##
==========================================
- Coverage   90.36%   89.72%   -0.65%     
==========================================
  Files          39       39              
  Lines        3437     3466      +29     
==========================================
+ Hits         3106     3110       +4     
- Misses        331      356      +25     
Impacted Files Coverage Δ
influxdb_client/client/bucket_api.py 58.46% <13.79%> (-35.99%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Pitastic
Copy link
Author

Pitastic commented Dec 28, 2022

(Sorry for the little mess. I merged my rebased master to early)

Thanks for your feedback. I really like your approach and implemented a version check (_is_below_v2()) and an attribute for this (self._build_version).

I use the docstrings and raise a DeprecationWarning when falling back to InfluxQL but I guess with well documented you mean more than that? Where can I add a little description for this behaviour?

@Pitastic Pitastic marked this pull request as ready for review December 29, 2022 19:46
@Pitastic Pitastic requested a review from bednar December 29, 2022 19:47
@Pitastic
Copy link
Author

Pitastic commented Jan 4, 2023

I don't know how to fix the semantic error in the PR title. Wozld you mind to change it to your needs, please?

@ZPascal
Copy link
Contributor

ZPascal commented Aug 4, 2023

Hi @Pitastic, I think it would be best to merge the commit and change the commit title to feat: Adding InfluxDB 1.8 support for database creation/deletion operations. I think it is also necessary to adjust the title of the PR to match that of the squashed commit.

If you need support, feel free to add me as a contributor to the original forked repository.

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.

None yet

4 participants