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

Added a common settings class to initialize plugin settings #361

Merged
merged 7 commits into from
Jan 5, 2024

Conversation

owaiskazi19
Copy link
Member

@owaiskazi19 owaiskazi19 commented Jan 4, 2024

Description

This PR achieves the below:

  • Moved initialization of settings to a common singleton class
  • Added tests

Note: Settings used in Rest classes works well as initialized by the REST call.

Testing:

[2024-01-04T18:50:05,940][INFO ][o.o.c.s.ClusterSettings  ] [ip-172-31-56-214] updating [plugins.flow_framework.max_get_task_request_retry] from [10] to [14]
[2024-01-04T19:07:37,941][INFO ][o.o.f.w.AbstractRetryableWorkflowStep] [ip-172-31-56-214] UPDATED RETRY SETTINGS 14

Issues Resolved

Fixes #351

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.

Signed-off-by: Owais Kazi <[email protected]>
Signed-off-by: owaiskazi19 <[email protected]>
Signed-off-by: Owais Kazi <[email protected]>
Signed-off-by: owaiskazi19 <[email protected]>
Signed-off-by: owaiskazi19 <[email protected]>
Copy link

codecov bot commented Jan 4, 2024

Codecov Report

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

Comparison is base (8216cb9) 72.52% compared to head (17c7489) 72.55%.

Files Patch % Lines
...ch/flowframework/workflow/WorkflowStepFactory.java 50.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main     #361      +/-   ##
============================================
+ Coverage     72.52%   72.55%   +0.03%     
- Complexity      570      572       +2     
============================================
  Files            73       72       -1     
  Lines          2984     2988       +4     
  Branches        226      226              
============================================
+ Hits           2164     2168       +4     
  Misses          716      716              
  Partials        104      104              

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

Copy link
Member

@dbwiddis dbwiddis left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@joshpalis joshpalis left a comment

Choose a reason for hiding this comment

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

LGTM as well, thanks @owaiskazi19

@owaiskazi19 owaiskazi19 merged commit a8e7ff5 into opensearch-project:main Jan 5, 2024
21 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jan 5, 2024
* Moved settings to a common singleton class

Signed-off-by: Owais Kazi <[email protected]>
Signed-off-by: owaiskazi19 <[email protected]>

* Removed update retry settings

Signed-off-by: Owais Kazi <[email protected]>

* Used getter for maxRetry value

Signed-off-by: Owais Kazi <[email protected]>
Signed-off-by: owaiskazi19 <[email protected]>

* Removed FeatureEnabledSettings file

Signed-off-by: Owais Kazi <[email protected]>

* Fixed tests

Signed-off-by: owaiskazi19 <[email protected]>

* Moved max workflow step setting to common class

Signed-off-by: Owais Kazi <[email protected]>

---------

Signed-off-by: Owais Kazi <[email protected]>
Signed-off-by: owaiskazi19 <[email protected]>
(cherry picked from commit a8e7ff5)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
dbwiddis pushed a commit that referenced this pull request Jan 5, 2024
…tings (#370)

Added a common settings class to initialize plugin settings (#361)

* Moved settings to a common singleton class




* Removed update retry settings



* Used getter for maxRetry value




* Removed FeatureEnabledSettings file



* Fixed tests



* Moved max workflow step setting to common class



---------



(cherry picked from commit a8e7ff5)

Signed-off-by: Owais Kazi <[email protected]>
Signed-off-by: owaiskazi19 <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x backport PRs to 2.x branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Max retry setting for the AbstractRetryableWorkflowStep doesn't update
3 participants