Skip to content

Conversation

@xorphitus
Copy link
Contributor

@xorphitus xorphitus commented May 24, 2024

Proposed Changes

Support bucket creation for InfluxDB Serverless.

NOTE: The API is incompatible with Cloud Dedicated database creation API. Therefore, probably Cloud Dedicated database creation should be implemented as another function, e.g., CreateDatabase.

Checklist

  • CHANGELOG.md updated
  • Rebased/mergeable
  • A test has been added if appropriate
  • Tests pass
  • Commit messages are conventional
  • Sign CLA (if not already signed)

@codecov
Copy link

codecov bot commented May 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.16%. Comparing base (f95a9ff) to head (13c3dc3).

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #86      +/-   ##
==========================================
- Coverage   83.34%   78.16%   -5.19%     
==========================================
  Files          12       13       +1     
  Lines        1009     1044      +35     
==========================================
- Hits          841      816      -25     
- Misses        138      199      +61     
+ Partials       30       29       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@powersj
Copy link

powersj commented May 24, 2024

Therefore, probably Cloud Dedicated database creation should be implemented as another function, e.g., CreateDatabase.

@bednar - I would prefer to see some sort of namespacing or separation in this API between the different products as the APIs are different. It could be really confusing for users if they have to figure out which function belongs to which product. Thoughts?

@bednar
Copy link
Member

bednar commented May 24, 2024

@powersj, You’re absolutely right. We need to consider how to incorporate this into our source base to be sure that it’s clear to users which product they are interacting with.

@xorphitus
Copy link
Contributor Author

@powersj @bednar Thank you for your comments. Should I wait for your namespacing design or should I come up with it by myself?

@bednar
Copy link
Member

bednar commented May 27, 2024

@xorphitus, please feel free to create suggestions for namespacing. We welcome your input!

@thulasirajkomminar
Copy link
Contributor

@xorphitus
I have some working version of library for cloud dedicated let me know once you come up with the namespaces.

@bednar
Copy link
Member

bednar commented May 28, 2024

@xorphitus, what are your thoughts on using a separate management directory along with a serverless.go file? We could name the functions as ServerlessCreateBucket, ServerlessUpdateBucket, and ServerlessListBuckets. Does this structure seem logical to you?

@xorphitus
Copy link
Contributor Author

@bednar

using a separate management directory along with a serverless.go file

Sounds clean, but I think management/serverless.go doesn't work, and an easy way is management_serverless.go with a flat directory structure. The bucket creation function that I implemented reuses influxdb3.Client's package private methods and fields, so they must belong to the same directory.

As for function names, I propose the following; struct for namespacing. What do you think about this?

// management_serverless.go
type ServerelessClient struct {
	client *Client
}

func NewServerlessClient(client *Client) *ServerelessClient {
	return &ServerelessClient{client: client}
}

func (c *ServerelessClient) CreateBucket(ctx context.Context, bucket *Bucket) error {
	// Delegates some operations to `c.Client`
}


// Caller code
client, err := influxdb3.New(...)

serverlessManager := NewServerlessClient(client)
serverlessManager.CreateBucket(...)
serverlessManager.UpdateBucket(...)
serverlessManager.ListBucket(...)

I considered it might be overkill, and prefixing that you suggested might be sufficient, but administration tasks have very different characters from data read/write operations, so it would be okay to make it a separated client struct.

The drawback is that users may have to have at most three clients, influxdb3.Client, Serveless manager, and Cloud Dedicated manager, in a case where they need to use both Serverless and CloudDedicated, e.g., for development and production environments. It may look awkward.

@bednar
Copy link
Member

bednar commented May 29, 2024

Your proposal looks great 👍. Let’s proceed as you suggested.

@xorphitus xorphitus marked this pull request as ready for review May 30, 2024 10:02
@xorphitus
Copy link
Contributor Author

@powersj @bednar It's ready for code reviewing. I've applied the namespacing and fixed CI errors.

@jamesbalcombe83
Copy link
Contributor

@powersj @bednar I'm looking into why the codecov report shows a reduction in coverage (-5.19%) for @xorphitus. It looks like the code that is indicated as no longer being covered should be handled by client_e2e_test.go . I tried to run those test in my local, but couldn't.

Can you fill use in on what are we missing to be able to verify that these lines are in still covered? Or can you see any reason why those lines are no longer covered based on the content of the PR?

@bednar
Copy link
Member

bednar commented Jun 11, 2024

Hi @xorphitus,

The end-to-end tests do not run for pull requests from third-party repositories to prevent exposing sensitive credentials. This limitation will be addressed once InfluxDB 3 OSS is released.

I will begin reviewing your contribution as soon as possible.

Best regards

@bednar
Copy link
Member

bednar commented Jun 11, 2024

@jamesbalcombe83, please see the comment above for more details: #86 (comment)

Copy link
Member

@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.

@xorphitus, thank you for your PR 👍. The API looks great so far. Please add a new example file for your API, similar to this existing example. This will assist users in understanding how to utilize your new API effectively. For detailed information on godoc, refer to this PR.

@xorphitus
Copy link
Contributor Author

@bednar I made an example file with 57ab218. Is this sufficient?

I also applied one more commit 13c3dc3 because I found I missed licence texts.

Copy link
Member

@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.

Thank again for your PR 👍

LGTM 🚀

@bednar bednar merged commit b4f432a into InfluxCommunity:main Jun 12, 2024
@bednar bednar added this to the 0.8.0 milestone Jun 12, 2024
@xorphitus xorphitus deleted the db-creation-api branch June 12, 2024 10:27
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.

5 participants