feat: Clickhouse Storage Driver #1342
Conversation
luk3skyw4lker
requested review from
gaby,
sixcolors,
ReneWerner87 and
efectn
and removed request for
a team
|
Warning Review failedThe pull request is closed. WalkthroughThe update introduces a Clickhouse storage driver for Go applications using the Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant ClickhouseDriver
participant ClickhouseDB
App->>ClickhouseDriver: New(config)
ClickhouseDriver-->>ClickhouseDB: Connect with config
ClickhouseDB-->>ClickhouseDriver: Connection Established
App->>ClickhouseDriver: Set(key, value, expiration)
ClickhouseDriver->>ClickhouseDB: Insert data
ClickhouseDB-->>ClickhouseDriver: Data Inserted
App->>ClickhouseDriver: Get(key)
ClickhouseDriver->>ClickhouseDB: Query data
ClickhouseDB-->>ClickhouseDriver: Return data
App->>ClickhouseDriver: Delete(key)
ClickhouseDriver->>ClickhouseDB: Delete data
ClickhouseDB-->>ClickhouseDriver: Data Deleted
App->>ClickhouseDriver: Reset()
ClickhouseDriver->>ClickhouseDB: Reset table
ClickhouseDB-->>ClickhouseDriver: Table Reset
App->>ClickhouseDriver: Close()
ClickhouseDriver->>ClickhouseDB: Close connection
ClickhouseDB-->>ClickhouseDriver: Connection Closed
Assessment against linked issues
Poem
Tip AI model upgrade
|
gaby
changed the title
|
Taking a look at the failing checks already |
|
@luk3skyw4lker The go.mod files are still broken. You edited the root one |
|
@gaby fixed it, sorry to take so long! |
|
@luk3skyw4lker The benchmark yml has to be updated to run clickhouse |
|
@gaby I added the clickhouse startup command, should be ok right now |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- clickhouse/README.md (1 hunks)
Additional context used
LanguageTool
clickhouse/README.md
[uncategorized] ~44-~44: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: ...ickhouse-server ``` After running this command you're ready to start using the storage...
Markdownlint
clickhouse/README.md
5-5: Expected: h2; Actual: h3 (MD001, heading-increment)
Heading levels should only increment by one level at a time
46-46: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
30-30: null (MD031, blanks-around-fences)
Fenced code blocks should be surrounded by blank lines
48-48: null (MD031, blanks-around-fences)
Fenced code blocks should be surrounded by blank lines
53-53: null (MD031, blanks-around-fences)
Fenced code blocks should be surrounded by blank lines
119-119: null (MD047, single-trailing-newline)
Files should end with a single newline character
Additional comments not posted (2)
clickhouse/README.md (2)
1-4: Header and introduction look good.The introduction succinctly describes the purpose of the storage driver and provides a useful link to the
clickhouse-golibrary.
25-44: Installation instructions are clear and detailed.The steps for installing the Clickhouse storage driver and setting up a local environment using Docker are well-documented and easy to follow.
Tools
LanguageTool
[uncategorized] ~44-~44: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: ...ickhouse-server ``` After running this command you're ready to start using the storage...Markdownlint
30-30: null (MD031, blanks-around-fences)
Fenced code blocks should be surrounded by blank lines
|
@luk3skyw4lker Run |
|
@gaby done! |
|
@luk3skyw4lker There's a issue somewhere in the implementation. The tests fail/pass based on timing |
|
@luk3skyw4lker I think callig driver.Context is sharing context between tests. I've seen this happen with Prometheus too. |
|
@gaby yeah, I'll have to check this later, tried some stuff but it didn't solve the issue, I'll try it tomorrow morning when I have the time |
|
It doesn't seems like the sharing context it's the issue tho, I removed all driver.Context calls and tested it with creating a context.Background() for each separately call and the same error was raised, I'm gonna keep checking it out while I have the time |
|
Alright, found out what was going on, even when the expiration value was equal the zero value of the time.Time struct, the IsZero function was returning false, so I changed the comparision and everything works out now. Committing the solution right this instant |
There was a problem hiding this comment.
Actionable comments posted: 7
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
clickhouse/go.sumis excluded by!**/*.sum,!**/*.sum
Files selected for processing (4)
- .github/dependabot.yml (1 hunks)
- clickhouse/clickhouse.go (1 hunks)
- clickhouse/clickhouse_test.go (1 hunks)
- clickhouse/config.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- .github/dependabot.yml
- clickhouse/config.go
Additional context used
Learnings (1)
clickhouse/clickhouse.go (1)
Learnt from: luk3skyw4lker PR: gofiber/storage#1342 File: clickhouse/clickhouse.go:57-58 Timestamp: 2024-05-09T23:43:16.810Z Learning: The pattern of returning `nil` for empty keys or values is consistent across different storage drivers in the gofiber/storage repository, including Redis, Rueidis, and MongoDB.
Additional comments not posted (13)
clickhouse/clickhouse.go (4)
121-123: LGTM!The
Resetfunction looks good and follows the expected logic.
125-127: LGTM!The
Closefunction looks good and follows the expected logic.
44-47: Handle the result of the Ping method.The
Pingmethod is called at the creation of aStorageinstance, but its result is not checked. This could lead to scenarios where an unresponsive or faulty connection is used, leading to further errors.err = conn.Ping(ctx) if err != nil { return nil, err }Likely invalid or redundant comment.
57-58: Ensure proper handling of empty keys and values.Returning
nilwhen key or value is empty might hide bugs. It's better to return an explicit error to notify the caller of the invalid input.- return nil + return fmt.Errorf("key and value must not be empty")Likely invalid or redundant comment.
clickhouse/clickhouse_test.go (9)
23-33: LGTM!The
Test_Connectionfunction looks good and follows the expected logic.
35-48: LGTM!The
Test_Setfunction looks good and follows the expected logic.
50-63: LGTM!The
Test_Set_With_Expfunction looks good and follows the expected logic.
65-84: LGTM!The
Test_Getfunction looks good and follows the expected logic.
86-112: LGTM!The
Test_Get_With_Expfunction looks good and follows the expected logic.
162-182: LGTM!The
Benchmark_Clickhouse_Setfunction looks good and follows the expected logic.
184-206: LGTM!The
Benchmark_Clickhouse_Getfunction looks good and follows the expected logic.
208-229: LGTM!The
Benchmark_Clickhouse_Set_And_Deletefunction looks good and follows the expected logic.
114-135: Add a test to ensure the deleted key is not retrievable.After deleting a key, you should verify that attempting to retrieve the key returns an expected result (e.g., key not found). This ensures that the delete operation functions correctly.
+ assert.Equal(t, []byte{}, value)This additional check ensures the delete functionality is robust and behaves as expected under normal conditions.
Likely invalid or redundant comment.
|
@gaby I addressed your changes and fixed the issues the driver was having, could you check again please? |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- clickhouse/clickhouse.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- clickhouse/clickhouse.go
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (8)
clickhouse/README.md (8)
1-1: Add a blank line after the header for proper markdown formatting.This ensures the document adheres to markdown best practices.
# Clickhouse +
5-5: Ensure each heading is followed by a blank line.This is a markdown formatting requirement to separate content visually and structurally.
### Table of Contents +Tools
Markdownlint
5-5: Expected: h2; Actual: h3
Heading levels should only increment by one level at a time(MD001, heading-increment)
13-13: Add blank lines around fenced code blocks.This improves readability and adheres to markdown best practices.
### Signatures + +
25-25: Correct markdown formatting around headings and code blocks.Ensure there are blank lines around headings and code blocks to meet markdown standards.
### Installation + +
30-30: Surround fenced code blocks with blank lines.This improves readability and adheres to markdown best practices.
Install the clickhouse implementation: + ```bash go get github.com/gofiber/storage/clickhouse
<details> <summary>Tools</summary> <details> <summary>Markdownlint</summary><blockquote> 30-30: null Fenced code blocks should be surrounded by blank lines (MD031, blanks-around-fences) </blockquote></details> </details> --- `44-44`: **Add a comma after introductory phrases.** This improves readability and adheres to grammatical best practices. ```diff After running this command you're ready to start using the storage and connecting to the database. + After running this command, you're ready to start using the storage and connecting to the database.Tools
LanguageTool
[uncategorized] ~44-~44: Possible missing comma found.
Context: ...ickhouse-server ``` After running this command you're ready to start using the storage...(AI_HYDRA_LEO_MISSING_COMMA)
49-49: Surround fenced code blocks with blank lines.This improves readability and adheres to markdown best practices.
You can use the following options to create a clickhouse storage driver: + ```go import "github.com/gofiber/storage/clickhouse"
<details> <summary>Tools</summary> <details> <summary>Markdownlint</summary><blockquote> 49-49: null Fenced code blocks should be surrounded by blank lines (MD031, blanks-around-fences) </blockquote></details> </details> --- `81-81`: **Replace hard tabs with spaces for consistency in markdown files.** The use of hard tabs in markdown files can lead to inconsistent formatting across different viewers and editors. ```diff - // The host of the database. Ex: 127.0.0.1 + // The host of the database. Ex: 127.0.0.1 - Host string + Host string - // The port where the database is supposed to listen to. Ex: 9000 + // The port where the database is supposed to listen to. Ex: 9000
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- clickhouse/README.md (1 hunks)
Additional context used
LanguageTool
clickhouse/README.md
[uncategorized] ~44-~44: Possible missing comma found.
Context: ...ickhouse-server ``` After running this command you're ready to start using the storage...(AI_HYDRA_LEO_MISSING_COMMA)
Markdownlint
clickhouse/README.md
5-5: Expected: h2; Actual: h3
Heading levels should only increment by one level at a time(MD001, heading-increment)
30-30: null
Fenced code blocks should be surrounded by blank lines(MD031, blanks-around-fences)
49-49: null
Fenced code blocks should be surrounded by blank lines(MD031, blanks-around-fences)
Adding clickhouse implementation.
Fixes #1204
Summary by CodeRabbit
New Features
Documentation
Tests
Chores