refactor: replace tempdir with tempfile by etolbakov · Pull Request #1123 · GreptimeTeam/greptimedb

etolbakov · 2023-03-03T22:55:02Z

I hereby agree to the terms of the GreptimeDB CLA

What's changed and what's your intention?

All usages of tempdir were replaced with tempfile.
tempdir has been removed from dependencies.

Checklist

I have written the necessary rustdoc comments.
I have added the necessary unit tests and integration tests.

Refer to a related PR or issue link (optional)

#1108

codecov · 2023-03-03T23:19:50Z

Codecov Report

Merging #1123 (f761030) into develop (bd065ea) will increase coverage by 0.03%.
The diff coverage is 100.00%.

❗ Current head f761030 differs from pull request most recent head 5dceed2. Consider uploading reports for the commit 5dceed2 to get more accurate results

@@             Coverage Diff             @@
##           develop    #1123      +/-   ##
===========================================
+ Coverage    85.02%   85.06%   +0.03%     
===========================================
  Files          474      476       +2     
  Lines        69073    69877     +804     
===========================================
+ Hits         58732    59442     +710     
- Misses       10341    10435      +94

etolbakov · 2023-03-03T23:29:20Z

Hi @evenyag
Could you please take a look when you have time?

killme2008

Great job, thanks a lot. Prefer moving the tempfile to the workspace's cargo.toml and refer it in every crate.

Jay-0331 · 2023-03-05T19:17:20Z

@etolbakov Hey a quick suggestion. Not sure if it is possible but I can see you have created a single function in some files can't we have a global file for the function and import it to other files and won't it be easier to modify in the future?

etolbakov · 2023-03-05T19:20:56Z

Hey @Jay-0331
Thanks for your feedback!
When I was working on this issue I had the same idea.
However, I'm relatively new to the codebase so if you could suggest what would be the best place for that function to be it would be great!
Maybe somewhere in src/common...

Jay-0331 · 2023-03-05T20:30:28Z

@etolbakov I am also new to the code actually 😅 but i was also finding the place where we can put the function came across the same folder src/common maybe @killme2008 could have a better idea on this

….toml

killme2008 · 2023-03-06T01:33:08Z

I think we can put the create_tmp_dir in the common/base crate. What do you think? @evenyag

killme2008

Almost LGTM. Only a little suggestion. Thanks.

waynexia · 2023-03-06T02:58:34Z

I think we can put the create_tmp_dir in the common/base crate. What do you think? @evenyag

common/base looks good to me. But to do so we need first add a feature gate to common/base, like the test feature in mito/Cargo.toml.

After this we need not depend on tempfile for every sub-crate but refer to the function in common/base.

evenyag

You could try this to specify the feature and optional dependency

https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#inheriting-a-dependency-from-a-workspace

evenyag · 2023-03-06T04:00:06Z

we need first add a feature gate to common/base, like the test feature in mito/Cargo.toml.

+1 for this.

etolbakov · 2023-03-06T06:29:13Z

Thanks for the hint, will give it a. try with test feature today

etolbakov · 2023-03-06T07:32:54Z

@evenyag
In order to reuse the create_tmp_dir function I plan to create the file common/base/src (f.e. temp_dir.rs) and make a corresponding change in common/base/src/lib.rs.
Once I'll make it, I can reference this function in other places like:
use common_base::temp_dir::create_tmp_dir;
Not sure I understood the necessity of the feature gate. Am I missing something?

evenyag · 2023-03-06T14:02:31Z

@evenyag In order to reuse the create_tmp_dir function I plan to create the file common/base/src (f.e. temp_dir.rs) and make a corresponding change in common/base/src/lib.rs. Once I'll make it, I can reference this function in other places like: use common_base::temp_dir::create_tmp_dir; Not sure I understood the necessity of the feature gate. Am I missing something?

We don't want to

import tempfile in non-tests dependencies (only in dev-dependencies)
expose test utils in public API

This is similar to the A test-only module method in this answer
https://stackoverflow.com/questions/44539729/what-is-an-idiomatic-way-to-have-shared-utility-functions-for-integration-tests

Another way is adding a new crate (maybe common/test-util) for test utilities. We could pub use tempfile in this crate. Looks like this is a recommended way.

etolbakov · 2023-03-06T23:17:22Z

@evenyag
I decided to go with the common/test-util approach. Could you please take a look when you have time?
Especially at log-store changes.
The only outstanding thing at the moment is mito as it uses the test-util in dependencies. Could you please give me a hint about if anything could be done there?

evenyag

You could re-export Tempdir in common-test-util. Then most crates don't need to import tempfile anymore.

Especially at log-store changes.

I'm ok with this. @v0y4g3r PTAL

etolbakov · 2023-03-07T20:22:40Z

@evenyag
I've made changes according to your suggestions, could you please have a look?
Btw, do not hesitate to add "nitpick" type of comments, I'm comfortable with that and I'm learning from you guys!

As far as I understood my build failed due to #1136

killme2008 · 2023-03-08T02:59:56Z

@evenyag PTAL

evenyag

LGTM

evenyag · 2023-03-08T03:15:18Z

Thanks for your patience! ❤️

Since the failed test should be unrelated to this PR, I plan to merge it now. @killme2008

* refactor: replace tempdir with tempfile * refactor(query): move tempfile dependency under the workspace's Cargo.toml * refactor(tempfile): create common-test-util * refactor(tempfile): fix toml format * refactor(tempfile): remove tempfile out of dependencies * refactor(tempfile): fix incorrect toml

refactor: replace tempdir with tempfile

d5ca573

killme2008 reviewed Mar 5, 2023

View reviewed changes

Comment thread src/common/catalog/Cargo.toml Outdated

refactor(query): move tempfile dependency under the workspace's Cargo…

842da72

….toml

killme2008 approved these changes Mar 6, 2023

View reviewed changes

Comment thread Cargo.toml Outdated

evenyag reviewed Mar 6, 2023

View reviewed changes

Comment thread src/mito/Cargo.toml Outdated

Comment thread src/mito/Cargo.toml Outdated

etolbakov added 2 commits March 6, 2023 22:45

refactor(tempfile): create common-test-util

0e91a63

refactor(tempfile): fix toml format

e4dd825

evenyag reviewed Mar 7, 2023

View reviewed changes

etolbakov added 2 commits March 7, 2023 19:19

refactor(tempfile): remove tempfile out of dependencies

b70bdc6

refactor(tempfile): fix incorrect toml

5dceed2

evenyag approved these changes Mar 8, 2023

View reviewed changes

evenyag merged commit b31a6cb into GreptimeTeam:develop Mar 8, 2023

etolbakov deleted the refactor/replace-tempdir-with-tempfile branch March 8, 2023 06:49

evenyag mentioned this pull request Mar 8, 2023

Replace tempdir with tempfile #1108

Closed

Conversation

etolbakov commented Mar 3, 2023

What's changed and what's your intention?

Checklist

Refer to a related PR or issue link (optional)

Uh oh!

codecov bot commented Mar 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

etolbakov commented Mar 3, 2023

Uh oh!

killme2008 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Jay-0331 commented Mar 5, 2023

Uh oh!

etolbakov commented Mar 5, 2023

Uh oh!

Jay-0331 commented Mar 5, 2023

Uh oh!

killme2008 commented Mar 6, 2023

Uh oh!

killme2008 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

waynexia commented Mar 6, 2023

Uh oh!

evenyag left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

evenyag commented Mar 6, 2023

Uh oh!

etolbakov commented Mar 6, 2023

Uh oh!

etolbakov commented Mar 6, 2023

Uh oh!

evenyag commented Mar 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

etolbakov commented Mar 6, 2023

Uh oh!

evenyag left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

etolbakov commented Mar 7, 2023

Uh oh!

killme2008 commented Mar 8, 2023

Uh oh!

evenyag left a comment

Choose a reason for hiding this comment

Uh oh!

evenyag commented Mar 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov bot commented Mar 3, 2023 •

edited

Loading

killme2008 left a comment •

edited

Loading

evenyag left a comment •

edited

Loading

evenyag commented Mar 6, 2023 •

edited

Loading

evenyag left a comment •

edited

Loading