Skip to content
This repository has been archived by the owner on Aug 9, 2022. It is now read-only.

Cleaning up settings name, Stats URL and redacting node details #35

Merged
merged 7 commits into from
Feb 14, 2021

Conversation

eirsep
Copy link
Contributor

@eirsep eirsep commented Feb 13, 2021

  1. Renames the settings to have opendistro.asynchronous_search
  2. Replaces _stats to stats
  3. Redacts the extra private details exposed by the stats
  4. Corrects throttling messages

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@codecov
Copy link

codecov bot commented Feb 13, 2021

Codecov Report

Merging #35 (c8b3804) into master (577a927) will decrease coverage by 0.12%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #35      +/-   ##
============================================
- Coverage     82.75%   82.63%   -0.13%     
+ Complexity      545      541       -4     
============================================
  Files            60       60              
  Lines          2053     2038      -15     
  Branches        149      145       -4     
============================================
- Hits           1699     1684      -15     
  Misses          245      245              
  Partials        109      109              
Impacted Files Coverage Δ Complexity Δ
.../asynchronous/plugin/AsynchronousSearchPlugin.java 100.00% <ø> (ø) 8.00 <0.00> (ø)
...ronous/rest/RestAsynchronousSearchStatsAction.java 45.45% <ø> (ø) 4.00 <0.00> (ø)
...ch/asynchronous/stats/AsynchronousSearchStats.java 92.30% <ø> (+17.30%) 6.00 <0.00> (-1.00) ⬆️
.../context/active/AsynchronousSearchActiveStore.java 85.71% <100.00%> (ø) 16.00 <3.00> (ø)
...anagement/AsynchronousSearchManagementService.java 77.44% <100.00%> (ø) 15.00 <0.00> (ø)
...synchronous/service/AsynchronousSearchService.java 92.40% <100.00%> (ø) 94.00 <0.00> (-1.00)
.../service/AsynchronousSearchPersistenceService.java 79.02% <0.00%> (-2.44%) 37.00% <0.00%> (-2.00%)
...ous/processor/AsynchronousSearchPostProcessor.java 71.42% <0.00%> (-1.30%) 14.00% <0.00%> (-1.00%)
...ous/listener/AsynchronousSearchTimeoutWrapper.java 86.20% <0.00%> (ø) 4.00% <0.00%> (ø%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 577a927...9ff0328. Read the comment docs.

Bukhtawar
Bukhtawar previously approved these changes Feb 13, 2021
@@ -40,7 +40,7 @@
private volatile int maxRunningSearches;
public static final int DEFAULT_MAX_RUNNING_SEARCHES = 20;
public static final Setting<Integer> MAX_RUNNING_SEARCHES_SETTING = Setting.intSetting(
"opendistro_asynchronous_search.max_running_searches", DEFAULT_MAX_RUNNING_SEARCHES, 0, Setting.Property.Dynamic,
"opendistro.asynchronous_search.max_running_searches", DEFAULT_MAX_RUNNING_SEARCHES, 0, Setting.Property.Dynamic,
Copy link
Member

Choose a reason for hiding this comment

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

Should we rename this to node.max_running_searches

Copy link
Member

Choose a reason for hiding this comment

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

node_concurrent_running_searches to follow other setting conventions

Copy link
Member

@Bukhtawar Bukhtawar left a comment

Choose a reason for hiding this comment

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

We also need to update README

@Bukhtawar Bukhtawar changed the title Settings rename Cleaning up settings name, Stats URL and redacting node details Feb 14, 2021
"opendistro_asynchronous_search.max_running_searches", DEFAULT_MAX_RUNNING_SEARCHES, 0, Setting.Property.Dynamic,
Setting.Property.NodeScope);
private volatile int nodeConcurrentRunningSearches;
public static final int NODE_CONCURRENT_RUNNING_SEARCHES = 20;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add something to variable name to signify this is max allowed per node ?

@Bukhtawar Bukhtawar merged commit e6322fb into master Feb 14, 2021
@Bukhtawar Bukhtawar deleted the settings-rename branch February 14, 2021 17:37
@Bukhtawar Bukhtawar restored the settings-rename branch February 14, 2021 17:37
@Bukhtawar Bukhtawar deleted the settings-rename branch February 14, 2021 17:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants