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

Treat Setting value with empty array string as empty array #10625

Merged

Conversation

machenity
Copy link
Contributor

@machenity machenity commented Oct 14, 2023

Description

According to the documentation, to create a dedicated coordinating node , empty array ([]) should be set as node.roles setting in the opensearch.yml document. The problem is that when I pass the setting value "[]"(env variables are always strings) via an environment variable, OpenSearch's environment variable parser does not determine it as an empty array and uses it as a string of "[]". As a result, it was not possible to create a dedicated coordinating node using an environment variable.

This PR fixes this so that if a listSetting value that is "[]" + any whitespaces, it is treated as an empty array.

Related Issues

Resolves #3412

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed per the DCO using --signoff
  • Commit changes are listed out in CHANGELOG.md file (See: Changelog)
  • Public documentation issue/PR created

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 14, 2023

Compatibility status:

Checks if related components are compatible with change 6f21088

Incompatible components

Incompatible components: [https://github.com/opensearch-project/performance-analyzer.git]

Skipped components

Compatible components

Compatible components: [https://github.com/opensearch-project/asynchronous-search.git, https://github.com/opensearch-project/security-analytics.git, https://github.com/opensearch-project/observability.git, https://github.com/opensearch-project/notifications.git, https://github.com/opensearch-project/reporting.git, https://github.com/opensearch-project/job-scheduler.git, https://github.com/opensearch-project/common-utils.git, https://github.com/opensearch-project/neural-search.git, https://github.com/opensearch-project/opensearch-oci-object-storage.git, https://github.com/opensearch-project/custom-codecs.git, https://github.com/opensearch-project/anomaly-detection.git, https://github.com/opensearch-project/geospatial.git, https://github.com/opensearch-project/ml-commons.git, https://github.com/opensearch-project/index-management.git, https://github.com/opensearch-project/security.git, https://github.com/opensearch-project/alerting.git, https://github.com/opensearch-project/cross-cluster-replication.git, https://github.com/opensearch-project/k-nn.git, https://github.com/opensearch-project/performance-analyzer-rca.git, https://github.com/opensearch-project/sql.git]

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@msfroh
Copy link
Collaborator

msfroh commented Oct 14, 2023

I'm concerned about the potential blast-radius of this change. Setting is used everywhere, so this may have unintended consequences. The string "[]" is not the same thing as the empty array.

Can we apply this logic only when reading settings from environment variables instead?

@machenity
Copy link
Contributor Author

machenity commented Oct 14, 2023

The change is in classListSetting, the subclass of Setting, and only applies to one of the static factory methods below it.
I think any settings which is an instance of ListSetting, expect its value to be either a yaml array or a string of comma-separated values.
The only settings that instantiated by that certain factor method are affected by the change, so I don't think the blast-radius would be that large.

Also, as far as I understand, the code that reads in the environment variable is located in ResolvePlaceholder, and I don't think it makes any checks or judgments about the content of the value of the retrieved environment variable. Rather, I'm concerned that reading the content, determining that it's an empty array of strings or something else and changing it might exceed its responsibility range.

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@machenity machenity force-pushed the ignore-empty-array-in-settings branch 2 times, most recently from 178bf08 to fe32d29 Compare October 18, 2023 15:25
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

machenity and others added 2 commits October 24, 2023 21:11
Signed-off-by: Jeongmin Yu <machenity@gmail.com>
…], ["a", "b", "c"], ...)

Signed-off-by: Andriy Redko <andriy.redko@aiven.io>
@machenity machenity force-pushed the ignore-empty-array-in-settings branch from fe32d29 to 8144ee7 Compare October 24, 2023 12:12
@github-actions github-actions bot added >breaking Identifies a breaking change. v3.0.0 Issues and PRs related to version 3.0.0 labels Oct 24, 2023
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      1 org.opensearch.search.SearchWeightedRoutingIT.testSearchAggregationWithNetworkDisruption_FailOpenEnabled

@codecov
Copy link

codecov bot commented Oct 24, 2023

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (3ff7e97) 71.15% compared to head (6f21088) 71.17%.
Report is 8 commits behind head on main.

Files Patch % Lines
.../java/org/opensearch/common/settings/Settings.java 77.77% 1 Missing and 3 partials ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main   #10625      +/-   ##
============================================
+ Coverage     71.15%   71.17%   +0.01%     
- Complexity    58783    58797      +14     
============================================
  Files          4885     4885              
  Lines        277199   277216      +17     
  Branches      40285    40290       +5     
============================================
+ Hits         197247   197308      +61     
- Misses        63448    63474      +26     
+ Partials      16504    16434      -70     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@reta
Copy link
Collaborator

reta commented Nov 15, 2023

@msfroh friendly ping please

Signed-off-by: Andriy Redko <andriy.redko@aiven.io>
Copy link
Contributor

❌ Gradle check result for 6f21088: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@reta reta mentioned this pull request Nov 15, 2023
16 tasks
Copy link
Contributor

❌ Gradle check result for 6f21088: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@reta
Copy link
Collaborator

reta commented Nov 15, 2023

❌ Gradle check result for 6f21088: FAILURE

#11208

Copy link
Contributor

✅ Gradle check result for 6f21088: SUCCESS

@reta reta merged commit 5b505ec into opensearch-project:main Nov 15, 2023
45 of 53 checks passed
fahadshamiinsta pushed a commit to fahadshamiinsta/OpenSearch270 that referenced this pull request Dec 4, 2023
…h-project#10625)

* Treat Setting value with empty array string as empty array

Signed-off-by: Jeongmin Yu <machenity@gmail.com>

* Allow to pass the list settings through environment variables (like [], ["a", "b", "c"], ...)

Signed-off-by: Andriy Redko <andriy.redko@aiven.io>

---------

Signed-off-by: Jeongmin Yu <machenity@gmail.com>
Signed-off-by: Andriy Redko <andriy.redko@aiven.io>
Co-authored-by: Andriy Redko <andriy.redko@aiven.io>
rayshrey pushed a commit to rayshrey/OpenSearch that referenced this pull request Mar 18, 2024
…h-project#10625)

* Treat Setting value with empty array string as empty array

Signed-off-by: Jeongmin Yu <machenity@gmail.com>

* Allow to pass the list settings through environment variables (like [], ["a", "b", "c"], ...)

Signed-off-by: Andriy Redko <andriy.redko@aiven.io>

---------

Signed-off-by: Jeongmin Yu <machenity@gmail.com>
Signed-off-by: Andriy Redko <andriy.redko@aiven.io>
Co-authored-by: Andriy Redko <andriy.redko@aiven.io>
shiv0408 pushed a commit to Gaurav614/OpenSearch that referenced this pull request Apr 25, 2024
…h-project#10625)

* Treat Setting value with empty array string as empty array

Signed-off-by: Jeongmin Yu <machenity@gmail.com>

* Allow to pass the list settings through environment variables (like [], ["a", "b", "c"], ...)

Signed-off-by: Andriy Redko <andriy.redko@aiven.io>

---------

Signed-off-by: Jeongmin Yu <machenity@gmail.com>
Signed-off-by: Andriy Redko <andriy.redko@aiven.io>
Co-authored-by: Andriy Redko <andriy.redko@aiven.io>
Signed-off-by: Shivansh Arora <hishiv@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>breaking Identifies a breaking change. bug Something isn't working v3.0.0 Issues and PRs related to version 3.0.0
Projects
None yet
3 participants