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

feat: add strict mode to validate protocol strings #3638

Merged
merged 14 commits into from
Apr 15, 2024

Conversation

irenjj
Copy link
Collaborator

@irenjj irenjj commented Apr 3, 2024

I hereby agree to the terms of the GreptimeDB CLA.

Refer to a related PR or issue link (optional)

#3435

What's changed and what's your intention?

provide a strict mode to perform UTF-8 validation

Checklist

  • I have written the necessary rustdoc comments.
  • I have added the necessary unit tests and integration tests.
  • This PR does not require documentation updates.

@github-actions github-actions bot added the docs-not-required This change does not impact docs. label Apr 3, 2024
@tisonkun tisonkun requested a review from v0y4g3r April 3, 2024 15:33
src/servers/src/http.rs Outdated Show resolved Hide resolved
@v0y4g3r
Copy link
Contributor

v0y4g3r commented Apr 9, 2024

Hi @irenjj I notice this draft is rather close to complete, can you add some tests for invalid UTF8 strings in labels and fix the CI workflows so that we can proceed?

@irenjj
Copy link
Collaborator Author

irenjj commented Apr 9, 2024

Hi @irenjj I notice this draft is rather close to complete, can you add some tests for invalid UTF8 strings in labels and fix the CI workflows so that we can proceed?

Sure, I'll take care of adding tests for invalid UTF8 strings right away.

@v0y4g3r v0y4g3r added A-write Involves code in write path A-metric-engine Involves code in metric engine labels Apr 10, 2024
@irenjj irenjj marked this pull request as ready for review April 11, 2024 11:46
@irenjj irenjj requested a review from a team as a code owner April 11, 2024 11:46
@killme2008
Copy link
Contributor

@v0y4g3r @etolbakov PTAL

src/servers/src/http.rs Outdated Show resolved Hide resolved
src/servers/src/http.rs Outdated Show resolved Hide resolved
@etolbakov
Copy link
Collaborator

Overall LGTM @irenjj, raised several points, will have another look (and approve) later today.

@tisonkun
Copy link
Collaborator

Fail test:

TRY 4 FAIL [ 1.139s] tests-integration::main integration_http_file_test::test_config_api

src/servers/benches/prom_decode.rs Show resolved Hide resolved
src/servers/src/http/prom_store.rs Show resolved Hide resolved
src/servers/src/prom_row_builder.rs Show resolved Hide resolved
src/servers/src/proto.rs Show resolved Hide resolved
src/servers/src/proto.rs Outdated Show resolved Hide resolved
@v0y4g3r v0y4g3r removed the docs-not-required This change does not impact docs. label Apr 12, 2024
@github-actions github-actions bot added the docs-not-required This change does not impact docs. label Apr 12, 2024
@v0y4g3r
Copy link
Contributor

v0y4g3r commented Apr 12, 2024

@irenjj Thanks, I've quickly walk through the PR and left some commet.

Despite these, you may also need to fix the test_config_api unit test which checks if runtime config retrieval API works as expected:

pub async fn test_config_api(store_type: StorageType) {

Also, the PR adds a config option to greptime binary, so would you mind adding to documentation for this option in greptimedb's doc repo so that it can be reflected in the official doc page. The related doc file is https://github.com/GreptimeTeam/docs/blob/main/docs/v0.7/en/user-guide/operations/configuration.md#protocol-options

@github-actions github-actions bot added docs-required This change requires docs update. and removed docs-not-required This change does not impact docs. labels Apr 12, 2024
@irenjj
Copy link
Collaborator Author

irenjj commented Apr 12, 2024

@irenjj Thanks, I've quickly walk through the PR and left some commet.

Despite these, you may also need to fix the test_config_api unit test which checks if runtime config retrieval API works as expected:

pub async fn test_config_api(store_type: StorageType) {

Also, the PR adds a config option to greptime binary, so would you mind adding to documentation for this option in greptimedb's doc repo so that it can be reflected in the official doc page. The related doc file is https://github.com/GreptimeTeam/docs/blob/main/docs/v0.7/en/user-guide/operations/configuration.md#protocol-options

Sure, i'll fix all review issues and add doc.

Copy link

codecov bot commented Apr 14, 2024

Codecov Report

Attention: Patch coverage is 62.90323% with 46 lines in your changes are missing coverage. Please review.

Project coverage is 85.22%. Comparing base (db329f6) to head (b24ac6f).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3638      +/-   ##
==========================================
- Coverage   85.57%   85.22%   -0.35%     
==========================================
  Files         962      962              
  Lines      160274   160371      +97     
==========================================
- Hits       137155   136683     -472     
- Misses      23119    23688     +569     

@v0y4g3r v0y4g3r self-requested a review April 15, 2024 06:35
Copy link
Contributor

@v0y4g3r v0y4g3r left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@waynexia waynexia left a comment

Choose a reason for hiding this comment

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

Others LGTM, thanks @irenjj, also thanks for your review @etolbakov @v0y4g3r

src/servers/tests/http/prom_store_test.rs Show resolved Hide resolved
@v0y4g3r v0y4g3r enabled auto-merge April 15, 2024 06:56
@v0y4g3r v0y4g3r added this pull request to the merge queue Apr 15, 2024
Merged via the queue into GreptimeTeam:main with commit 7a04bfe Apr 15, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-metric-engine Involves code in metric engine A-write Involves code in write path docs-required This change requires docs update.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants