feat(query): get the system open file limits and set #8388

TszKitLo40 · 2022-10-21T06:27:33Z

I hereby agree to the terms of the CLA available at: https://databend.rs/dev/policies/cla/

Summary

If the open file limit is less than 65535, set it to 65535.

vercel · 2022-10-21T06:27:38Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment

Name	Status	Preview	Updated
databend	⬜️ Ignored (Inspect)		Oct 26, 2022 at 5:55AM (UTC)

src/binaries/query/main.rs

BohuTANG · 2022-10-23T01:06:24Z

@mergify update

mergify · 2022-10-23T01:06:58Z

update

❌ Base branch update has failed

refusing to allow a GitHub App to create or update workflow .github/workflows/build-tool.yml without workflows permission
err-code: EE06A

BohuTANG · 2022-10-23T01:37:14Z

I have pushed a demo FYI:
BohuTANG@c6fb934

TszKitLo40 · 2022-10-23T09:05:57Z

I have pushed a demo FYI: BohuTANG@c6fb934

updated

TszKitLo40 · 2022-10-24T03:05:03Z

@BohuTANG It seems that limits-rs can't work on MacOS.

BohuTANG · 2022-10-24T03:17:09Z

@mergify update

mergify · 2022-10-24T03:17:44Z

update

❌ Base branch update has failed

refusing to allow a GitHub App to create or update workflow .github/workflows/build-tool.yml without workflows permission
err-code: 6EA3F

BohuTANG · 2022-10-25T00:03:52Z

@BohuTANG It seems that limits-rs can't work on MacOS.

We can add:

#[cfg(not(target_os = "macos"))]

to check.

Xuanwo · 2022-10-25T00:48:12Z

I wouldn't say I like this PR.

It's not a good idea to set open file limits in code. How about printing a warning message with the action needs to take?

For example:

The open file limit is too low for the databend-query. Please consider increase it by running `ulimit -n 65536`

There are many system settings that need to tune for query, we can't set them all.

sundy-li · 2022-10-26T01:46:59Z

How about printing a warning message with the action needs to take?

That's right!

TszKitLo40 · 2022-10-26T02:32:22Z

How about printing a warning message with the action needs to take?

That's right!

OK，I will print a warning message instead of set open file limits in this PR.

src/binaries/query/main.rs

Xuanwo · 2022-10-26T04:51:43Z

@TszKitLo40 Thanks for your work! This PR is almost LGTM now. I left some review here, PTAL.

Xuanwo

Thanks for your contribution!

Xuanwo · 2022-10-26T05:33:58Z

CI failure need to be addressed:

error[E0425]: cannot find value `e` in this scope
   --> src/binaries/query/main.rs:265:64
    |
265 |             warn!("get system limit of databend-query failed: {e:?}");
    |                                                                ^ not found in this scope

TszKitLo40 · 2022-10-26T05:41:38Z

CI failure need to be addressed:

error[E0425]: cannot find value `e` in this scope
   --> src/binaries/query/main.rs:265:64
    |
265 |             warn!("get system limit of databend-query failed: {e:?}");
    |                                                                ^ not found in this scope

I think I have fixed it.

Xuanwo · 2022-10-26T05:45:06Z

Please merge with the main branch to make sure our PR are updated~

BohuTANG · 2022-10-26T07:45:45Z

src/binaries/query/main.rs

@@ -34,7 +34,9 @@ use databend_query::servers::MySQLHandler;
 use databend_query::servers::Server;
 use databend_query::servers::ShutdownHandle;
 use databend_query::GlobalServices;
+use limits_rs::get_own_limits;


Remove the use.

BohuTANG · 2022-10-26T07:46:00Z

src/binaries/query/main.rs

 use tracing::info;
+use tracing::warn;


Remove the use.

BohuTANG · 2022-10-26T07:46:28Z

src/binaries/query/main.rs

+    let max_open_files_limit = limits.max_open_files.soft;
+    if let Some(max_open_files) = max_open_files_limit {
+        if max_open_files < 65535 {
+            warn!(


warn! --> tracing::warn!

BohuTANG · 2022-10-26T07:47:19Z

This PR breaks the MacOS:

error: unused import: `limits_rs::get_own_limits`
  --> src/binaries/query/main.rs:37:5
   |
37 | use limits_rs::get_own_limits;
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: `-D unused-imports` implied by `-D warnings`

error: unused import: `tracing::warn`
  --> src/binaries/query/main.rs:39:5
   |
39 | use tracing::warn;
   |     ^^^^^^^^^^^^^

BohuTANG · 2022-10-26T07:48:18Z

src/binaries/query/main.rs

+
+#[cfg(not(target_os = "macos"))]
+fn check_max_open_files() {
+    let limits = match get_own_limits() {


get_own_limits-->limits_rs::get_own_limits

Do I need to file another PR to fix this problems?

This works on my MacOS.

mergify bot added the pr-feature this PR introduces a new feature to the codebase label Oct 21, 2022

TszKitLo40 force-pushed the open-files-limit branch 3 times, most recently from 850fd44 to 1983e4f Compare October 21, 2022 07:31

TszKitLo40 marked this pull request as ready for review October 21, 2022 08:54

BohuTANG self-requested a review October 22, 2022 01:46

BohuTANG reviewed Oct 22, 2022

View reviewed changes

src/binaries/query/main.rs Outdated Show resolved Hide resolved

BohuTANG reviewed Oct 22, 2022

View reviewed changes

src/binaries/query/main.rs Outdated Show resolved Hide resolved

TszKitLo40 force-pushed the open-files-limit branch from 1983e4f to 8ed1d16 Compare October 23, 2022 01:21

TszKitLo40 force-pushed the open-files-limit branch from 8ed1d16 to 745242e Compare October 23, 2022 09:04

TszKitLo40 force-pushed the open-files-limit branch 2 times, most recently from df40ecd to 95d902e Compare October 24, 2022 02:43

TszKitLo40 force-pushed the open-files-limit branch from 95d902e to 9a20a4e Compare October 24, 2022 04:27

TszKitLo40 force-pushed the open-files-limit branch from 9a20a4e to 58a65e1 Compare October 25, 2022 00:46

Xuanwo reviewed Oct 26, 2022

View reviewed changes

src/binaries/query/main.rs Outdated Show resolved Hide resolved

src/binaries/query/main.rs Outdated Show resolved Hide resolved

Xuanwo approved these changes Oct 26, 2022

View reviewed changes

PsiACE approved these changes Oct 26, 2022

View reviewed changes

TszKitLo40 force-pushed the open-files-limit branch from 05668dc to d618911 Compare October 26, 2022 05:25

TszKitLo40 force-pushed the open-files-limit branch from d618911 to df1903a Compare October 26, 2022 05:36

TszKitLo40 added 3 commits October 26, 2022 13:54

feat(query): get the system open file limits and set

39fa62c

print warn message

0849c6c

Address review comments

5ed1855

TszKitLo40 force-pushed the open-files-limit branch from df1903a to 5ed1855 Compare October 26, 2022 05:55

mergify bot merged commit 2430726 into databendlabs:main Oct 26, 2022

BohuTANG reviewed Oct 26, 2022

View reviewed changes

src/binaries/query/main.rs

use tracing::info;

use tracing::warn;

Copy link

Member

BohuTANG Oct 26, 2022

Choose a reason for hiding this comment

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

Remove the use.

BohuTANG reviewed Oct 26, 2022

View reviewed changes

BohuTANG mentioned this pull request Nov 4, 2022

feat: get the system open file limits and set #8374

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(query): get the system open file limits and set #8388

feat(query): get the system open file limits and set #8388

TszKitLo40 commented Oct 21, 2022 •

edited

Loading

vercel bot commented Oct 21, 2022 •

edited

Loading

BohuTANG commented Oct 23, 2022

mergify bot commented Oct 23, 2022

BohuTANG commented Oct 23, 2022

TszKitLo40 commented Oct 23, 2022

TszKitLo40 commented Oct 24, 2022

BohuTANG commented Oct 24, 2022

mergify bot commented Oct 24, 2022

BohuTANG commented Oct 25, 2022

Xuanwo commented Oct 25, 2022 •

edited

Loading

sundy-li commented Oct 26, 2022

TszKitLo40 commented Oct 26, 2022

Xuanwo commented Oct 26, 2022

Xuanwo left a comment

Xuanwo commented Oct 26, 2022

TszKitLo40 commented Oct 26, 2022

Xuanwo commented Oct 26, 2022

BohuTANG Oct 26, 2022 •

edited

Loading

BohuTANG Oct 26, 2022

BohuTANG Oct 26, 2022

BohuTANG commented Oct 26, 2022

BohuTANG Oct 26, 2022

TszKitLo40 Oct 26, 2022

BohuTANG Oct 26, 2022

feat(query): get the system open file limits and set #8388

feat(query): get the system open file limits and set #8388

Conversation

TszKitLo40 commented Oct 21, 2022 • edited Loading

Summary

vercel bot commented Oct 21, 2022 • edited Loading

BohuTANG commented Oct 23, 2022

mergify bot commented Oct 23, 2022

❌ Base branch update has failed

BohuTANG commented Oct 23, 2022

TszKitLo40 commented Oct 23, 2022

TszKitLo40 commented Oct 24, 2022

BohuTANG commented Oct 24, 2022

mergify bot commented Oct 24, 2022

❌ Base branch update has failed

BohuTANG commented Oct 25, 2022

Xuanwo commented Oct 25, 2022 • edited Loading

sundy-li commented Oct 26, 2022

TszKitLo40 commented Oct 26, 2022

Xuanwo commented Oct 26, 2022

Xuanwo left a comment

Choose a reason for hiding this comment

Xuanwo commented Oct 26, 2022

TszKitLo40 commented Oct 26, 2022

Xuanwo commented Oct 26, 2022

BohuTANG Oct 26, 2022 • edited Loading

Choose a reason for hiding this comment

BohuTANG Oct 26, 2022

Choose a reason for hiding this comment

BohuTANG Oct 26, 2022

Choose a reason for hiding this comment

BohuTANG commented Oct 26, 2022

BohuTANG Oct 26, 2022

Choose a reason for hiding this comment

TszKitLo40 Oct 26, 2022

Choose a reason for hiding this comment

BohuTANG Oct 26, 2022

Choose a reason for hiding this comment

TszKitLo40 commented Oct 21, 2022 •

edited

Loading

vercel bot commented Oct 21, 2022 •

edited

Loading

Xuanwo commented Oct 25, 2022 •

edited

Loading

BohuTANG Oct 26, 2022 •

edited

Loading