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

Validate max number of node connections #196908

Merged

Conversation

SoniaSanzV
Copy link
Contributor

@SoniaSanzV SoniaSanzV commented Oct 18, 2024

#Closes #110155

Summary

Currently Elastic Search has a limitation of 2^31-1 (2147483647) maximum node connections. This PR adds this hardcoded value and is used to validate that the input does not exceed this value and, therefore, the user does not have to wait for the server to return the error to know that they have entered a number that is too high. ES does not have an API to query the number, that's why it is hardcoded.

Screenshot 2024-10-18 at 17 13 14

@SoniaSanzV SoniaSanzV added Feature:CCR and Remote Clusters Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more release_note:skip Skip the PR/issue when compiling release notes v9.0.0 backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) labels Oct 18, 2024
@SoniaSanzV SoniaSanzV self-assigned this Oct 18, 2024
@SoniaSanzV SoniaSanzV requested a review from a team as a code owner October 18, 2024 15:21
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-management (Team:Kibana Management)

@SoniaSanzV SoniaSanzV marked this pull request as draft October 18, 2024 15:21
@SoniaSanzV SoniaSanzV marked this pull request as ready for review October 18, 2024 15:29
@@ -33261,6 +33261,7 @@
"xpack.remoteClusters.form.errors.illegalCharacters": "从名称中删除{characterListLength, plural, other {字符}} {characterList}。",
"xpack.remoteClusters.form.errors.illegalSpace": "名称中不允许使用空格。",
"xpack.remoteClusters.form.errors.nameMissing": "“名称”必填。",
"xpack.remoteClusters.form.errors.maxValue": "此数字必须等于或小于 {maxValue}。",
Copy link
Contributor

Choose a reason for hiding this comment

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

How do we have the translations already?

Copy link
Contributor Author

@SoniaSanzV SoniaSanzV Oct 21, 2024

Choose a reason for hiding this comment

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

i reused the ones from Ingest_pipelines ("xpack.ingestPipelines.pipelineEditor.communityId.seedMaxNumberError"). I forgot to mention it in the description of the PR.

import React from 'react';
import { FormattedMessage } from '@kbn/i18n-react';

export const MAX_NODE_CONNECTIONS = 2147483647;
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to find a central place to keep this so it can also be referenced from tests

Copy link
Contributor

Choose a reason for hiding this comment

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

It could also be written (2 ** 31) - 1 instead of the calculated value, up to you.

Copy link
Contributor Author

@SoniaSanzV SoniaSanzV Oct 21, 2024

Choose a reason for hiding this comment

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

Agree about the file to place the constant. I'll find it.

About how to write the value, I decided to write the calculated value because I thought it was a better reference for manual testing, but it can also be specified in a comment.

// Show errors if there is a general form error or local errors.
const areFormErrorsVisible = Boolean(areErrorsVisible && seedsError);
const showErrors = areFormErrorsVisible || localSeedErrors.length !== 0;
const errors =
const showLocalSheedErrors = areFormErrorsVisible || localSeedErrors.length !== 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const showLocalSheedErrors = areFormErrorsVisible || localSeedErrors.length !== 0;
const showLocalSeedErrors = areFormErrorsVisible || localSeedErrors.length !== 0;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, this typo needs to be corrected.

@@ -107,8 +109,8 @@ export const SniffConnection: FunctionComponent<Props> = ({
}}
/>
}
isInvalid={showErrors}
error={errors}
isInvalid={showLocalSheedErrors}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
isInvalid={showLocalSheedErrors}
isInvalid={showLocalSeedErrors}

@@ -125,7 +127,7 @@ export const SniffConnection: FunctionComponent<Props> = ({
onFieldsChange({ seeds: options.map(({ label }) => label) })
}
onSearchChange={onSeedsInputChange}
isInvalid={showErrors}
isInvalid={showLocalSheedErrors}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
isInvalid={showLocalSheedErrors}
isInvalid={showLocalSeedErrors}

Copy link
Contributor

@mattkime mattkime left a comment

Choose a reason for hiding this comment

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

Code works well, made some minor suggestions that I'll leave for you do decide if you want to address or not.

@SoniaSanzV SoniaSanzV force-pushed the remoteClusters/maxSeedValidation_#110155 branch from 2fcb0c3 to 76e46b7 Compare October 21, 2024 07:30
@SoniaSanzV SoniaSanzV force-pushed the remoteClusters/maxSeedValidation_#110155 branch from 76e46b7 to d101881 Compare October 21, 2024 13:18
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
remoteClusters 124 125 +1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
remoteClusters 78.8KB 79.2KB +366.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
remoteClusters 9.1KB 9.2KB +44.0B

History

  • 💚 Build #244126 succeeded 76e46b799b55f0cb52903cbfd41c1725c344ff14

cc @SoniaSanzV

@SoniaSanzV SoniaSanzV merged commit 860e445 into elastic:main Oct 21, 2024
23 checks passed
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/11444394688

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Oct 21, 2024
#Closes [elastic#110155](elastic#110155)

## Summary
Currently Elastic Search has a limitation of 2^31-1 (2147483647) maximum
node connections. This PR adds this hardcoded value and is used to
validate that the input does not exceed this value and, therefore, the
user does not have to wait for the server to return the error to know
that they have entered a number that is too high. ES does not have an
API to query the number, that's why it is hardcoded.

<img width="1500" alt="Screenshot 2024-10-18 at 17 13 14"
src="https://github.com/user-attachments/assets/c18a4756-df76-4e0e-ba31-5118208f686d">

(cherry picked from commit 860e445)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Oct 21, 2024
# Backport

This will backport the following commits from `main` to `8.x`:
- [Validate max number of node connections
(#196908)](#196908)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Sonia Sanz
Vivas","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-10-21T16:21:02Z","message":"Validate
max number of node connections (#196908)\n\n#Closes
[#110155](https://github.com/elastic/kibana/issues/110155)\r\n\r\n##
Summary\r\nCurrently Elastic Search has a limitation of 2^31-1
(2147483647) maximum\r\nnode connections. This PR adds this hardcoded
value and is used to\r\nvalidate that the input does not exceed this
value and, therefore, the\r\nuser does not have to wait for the server
to return the error to know\r\nthat they have entered a number that is
too high. ES does not have an\r\nAPI to query the number, that's why it
is hardcoded.\r\n\r\n<img width=\"1500\" alt=\"Screenshot 2024-10-18 at
17 13
14\"\r\nsrc=\"https://github.com/user-attachments/assets/c18a4756-df76-4e0e-ba31-5118208f686d\">","sha":"860e445c7d6a7d5bb92a638e5ac5cb888f881ffc","branchLabelMapping":{"^v9.0.0$":"main","^v8.17.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Feature:CCR
and Remote Clusters","Team:Kibana
Management","release_note:skip","v9.0.0","backport:prev-minor"],"title":"Validate
max number of node
connections","number":196908,"url":"https://github.com/elastic/kibana/pull/196908","mergeCommit":{"message":"Validate
max number of node connections (#196908)\n\n#Closes
[#110155](https://github.com/elastic/kibana/issues/110155)\r\n\r\n##
Summary\r\nCurrently Elastic Search has a limitation of 2^31-1
(2147483647) maximum\r\nnode connections. This PR adds this hardcoded
value and is used to\r\nvalidate that the input does not exceed this
value and, therefore, the\r\nuser does not have to wait for the server
to return the error to know\r\nthat they have entered a number that is
too high. ES does not have an\r\nAPI to query the number, that's why it
is hardcoded.\r\n\r\n<img width=\"1500\" alt=\"Screenshot 2024-10-18 at
17 13
14\"\r\nsrc=\"https://github.com/user-attachments/assets/c18a4756-df76-4e0e-ba31-5118208f686d\">","sha":"860e445c7d6a7d5bb92a638e5ac5cb888f881ffc"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/196908","number":196908,"mergeCommit":{"message":"Validate
max number of node connections (#196908)\n\n#Closes
[#110155](https://github.com/elastic/kibana/issues/110155)\r\n\r\n##
Summary\r\nCurrently Elastic Search has a limitation of 2^31-1
(2147483647) maximum\r\nnode connections. This PR adds this hardcoded
value and is used to\r\nvalidate that the input does not exceed this
value and, therefore, the\r\nuser does not have to wait for the server
to return the error to know\r\nthat they have entered a number that is
too high. ES does not have an\r\nAPI to query the number, that's why it
is hardcoded.\r\n\r\n<img width=\"1500\" alt=\"Screenshot 2024-10-18 at
17 13
14\"\r\nsrc=\"https://github.com/user-attachments/assets/c18a4756-df76-4e0e-ba31-5118208f686d\">","sha":"860e445c7d6a7d5bb92a638e5ac5cb888f881ffc"}}]}]
BACKPORT-->

Co-authored-by: Sonia Sanz Vivas <[email protected]>
@SoniaSanzV SoniaSanzV deleted the remoteClusters/maxSeedValidation_#110155 branch October 25, 2024 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) Feature:CCR and Remote Clusters release_note:skip Skip the PR/issue when compiling release notes Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v8.17.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Remote Clusters] Error Callout Has Wrong Icon and Color
4 participants