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.

Sign up for GitHub

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

Jump to bottom

Refactor handler method #158

Merged
merged 21 commits into from
Sep 30, 2022
Merged

Refactor handler method #158

merged 21 commits into from
Sep 30, 2022

Conversation

mloufra
Copy link
Contributor

@mloufra mloufra commented Sep 26, 2022

Description

Create the following five new classes to handle each handler method for refactoring the class in ExtensionsRunner.java.

  • ExtensionsInitRequestHandler handle method handleExtensionInitRequest
  • OpensearchRequestHandler handle method handleOpenSearchRequest
  • ExtensionsIndicesModuleRequestHandler handle method handleIndicesModuleRequest
  • ExtensionsIndicesModuleNameRequestHandler handle method handleIndicesModuleNameRequest
  • ExtenionsRestRequestHandler handle method handleRestExecuteOnExtensionRequest

Issues Resolved

#116

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.

mloufra and others added 12 commits August 26, 2022 16:43
@mloufra
issue #28
7502353 
Signed-off-by: mloufra <[email protected]>
@mloufra
Update the lastest coomit
dbb47f5 
Signed-off-by: mloufra <[email protected]>
@mloufra
Rename the method and fix the conflict
bea6873 
Signed-off-by: mloufra <[email protected]>
@mloufra
fix merge conflict
0e5dd7f 
Signed-off-by: mloufra <[email protected]>
@mloufra
Add code coverage report
11bd149 
Signed-off-by: mloufra <[email protected]>
@mloufra
Rebase the lastest commit
bbccd26 
Signed-off-by: mloufra <[email protected]>
@mloufra
updata the lastest commit
b182b35 
Signed-off-by: mloufra <[email protected]>
@mloufra
update the lastest commit
1bcb46c 
Signed-off-by: mloufra <[email protected]>
@mloufra
Merge branch 'opensearch-project:main' into main
50ff5e7
@mloufra
Merge branch 'main' of https://github.com/mloufra/opensearch-sdk into…
2309183 
… main
@mloufra
Merge branch 'main' of https://github.com/mloufra/opensearch-sdk into…
05afe09 
… main
@mloufra
Merge branch 'main' of https://github.com/mloufra/opensearch-sdk into…
cdac476 
… main
@mloufra mloufra requested a review from a team September 26, 2022 23:19
@mloufra mloufra mentioned this pull request Sep 26, 2022
Closed
dbwiddis
dbwiddis requested changes Sep 27, 2022
View reviewed changes
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.

I'm not sure why so many variables were changed to static, and suspect there's a better way to handle whatever challenge that was meant to address.

src/main/java/org/opensearch/sdk/ExtensionsRunner.java Outdated Show resolved Hide resolved
src/main/java/org/opensearch/sdk/ExtensionsRunner.java Outdated Show resolved Hide resolved
src/main/java/org/opensearch/sdk/handlers/ExtensionsRestRequestHandler.java Outdated Show resolved Hide resolved
src/main/java/org/opensearch/sdk/handlers/OpensearchRequestHandler.java Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Sep 28, 2022 •
edited
Loading

Codecov Report

Merging #158 (6d690fb) into main (d7b873c) will increase coverage by 0.90%.
The diff coverage is 54.23%.

@@             Coverage Diff              @@
##               main     #158      +/-   ##
============================================
+ Coverage     63.69%   64.60%   +0.90%     
- Complexity       90       99       +9     
============================================
  Files            20       25       +5     
  Lines           482      500      +18     
  Branches         18       18              
============================================
+ Hits            307      323      +16     
- Misses          167      169       +2     
  Partials          8        8
Impacted Files Coverage Δ
.../java/org/opensearch/sdk/ExtensionRestRequest.java 56.25% <ø> (ø)
...rch/sdk/handlers/ExtensionsRestRequestHandler.java 33.33% <33.33%> (ø)
...ers/ExtensionsIndicesModuleNameRequestHandler.java 40.00% <40.00%> (ø)
...andlers/ExtensionsIndicesModuleRequestHandler.java 40.00% <40.00%> (ø)
...main/java/org/opensearch/sdk/ExtensionsRunner.java 67.44% <50.00%> (+4.62%) ⬆️
...nsearch/sdk/handlers/OpensearchRequestHandler.java 60.00% <60.00%> (ø)
...rch/sdk/handlers/ExtensionsInitRequestHandler.java 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@mloufra mloufra requested a review from dbwiddis September 28, 2022 22:10
owaiskazi19
owaiskazi19 reviewed Sep 29, 2022
View reviewed changes
src/main/java/org/opensearch/sdk/handlers/OpensearchRequestHandler.java
* @return A response to OpenSearch for the corresponding API
* @throws Exception if the corresponding handler for the request is not present
*/
public TransportResponse handleOpenSearchRequest(OpenSearchRequest request) throws Exception {
Copy link
Member

Choose a reason for hiding this comment

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

This should be a class for NamedWriteableRegistry and not OpenSearchRequest. Also, we don't need a switch case here. If/else can do the job.

Copy link
Member

Choose a reason for hiding this comment

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

It used to have a lot more cases before we refactored :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can keep this switch case here, it will easier for others adding more case in the future.

dbwiddis
dbwiddis reviewed Sep 29, 2022
View reviewed changes
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.

Looking good with the documentation comments noted!

src/main/java/org/opensearch/sdk/ExtensionsRunner.java Outdated Show resolved Hide resolved
src/main/java/org/opensearch/sdk/ExtensionsRunner.java Outdated Show resolved Hide resolved
src/main/java/org/opensearch/sdk/ExtensionsRunner.java Show resolved Hide resolved
src/main/java/org/opensearch/sdk/handlers/ExtensionsRestRequestHandler.java Show resolved Hide resolved
src/main/java/org/opensearch/sdk/handlers/OpensearchRequestHandler.java
* @return A response to OpenSearch for the corresponding API
* @throws Exception if the corresponding handler for the request is not present
*/
public TransportResponse handleOpenSearchRequest(OpenSearchRequest request) throws Exception {
Copy link
Member

Choose a reason for hiding this comment

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

It used to have a lot more cases before we refactored :-)

@dbwiddis dbwiddis mentioned this pull request Sep 29, 2022
Merged
dbwiddis
dbwiddis approved these changes Sep 30, 2022
View reviewed changes
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! Any additional requests please put on the linked issue for the next round, so we can merge this. :)

@dbwiddis dbwiddis merged commit 0bdd6b4 into opensearch-project:main Sep 30, 2022
@dbwiddis dbwiddis deleted the refactor-handler branch September 30, 2022 02:50
kokibas pushed a commit to kokibas/opensearch-sdk-java that referenced this pull request Mar 17, 2023
@mloufra @kokibas
Refactor handler method (opensearch-project#158)
bfaf46e 
* issue opensearch-project#28

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

* Update the lastest coomit

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

* Rename the method and fix the conflict

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

* fix merge conflict

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

* Add code coverage report

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

* Rebase the lastest commit

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

* update the lastest commit

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

* refactor class for handler method in ExtensionsRunner

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

* add documentation for handler

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

* fix merge conflict

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

* delete all the static

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

* fix documentation problem

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

* fix NullPointerException bug

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

* change RestResponse to ExtensionRestResponse in ExtensionsRestRequestHandler

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

* change documentation for ExtensionsRunner

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

Signed-off-by: mloufra <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@owaiskazi19 owaiskazi19 owaiskazi19 approved these changes

@dbwiddis dbwiddis dbwiddis approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mloufra @codecov-commenter @dbwiddis @owaiskazi19