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

Refactor handler interface and LuceneServer class #743

Merged
merged 6 commits into from
Oct 3, 2024

Conversation

sarthakn7
Copy link
Contributor

We currently have the logic for grpc methods split across LuceneServer and the individual handler interfaces for each method. There are several issues here:

  1. Some methods don't need/have handlers
  2. There is no way of telling what logic should be in LuceneServer or in handler for a single rpc
  3. The handler interface is restrictive since it only allows IndexState and the request to be passed to the handle function. We need to create a constructor to pass any additional parameters.
  4. We create handler objects for every call. These can easily be singletons.

Changes made:

  1. Create a new Handler abstract class under handler package which is initialized with GlobalState and has methods for unary and streaming rpc handling which take the request proto and response observer
  2. All the methods under LuceneServerImpl migrated to use these handlers
  3. Handlers created under handler package for any methods which did not already have a handler
  4. Handlers are created once during initialization now
  5. Handler interface is still there but only used by ReplicationServer methods

Notes:

  1. I first tried making handler instances static, but it was causing test failures since we create multiple servers in tests.
  2. I started out with making globalState a variable in the handler, but later thought that it doesn't need to be. I can remove it from the abstract class right now if there is a strong preference, otherwise I'll let it stay.
  3. Future PRs will move other Handler classes into the handler package.

@sarthakn7 sarthakn7 marked this pull request as ready for review October 3, 2024 01:28
@sarthakn7 sarthakn7 requested a review from aprudhomme October 3, 2024 01:28
Comment on lines 95 to 96
public SearchHandler(ThreadPoolExecutor threadPoolExecutor, boolean warming) {
super(null);
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be better to pass the global state here to keep the instance fully functional

…dlers

# Conflicts:
#	src/main/java/com/yelp/nrtsearch/server/luceneserver/SearchHandler.java
@sarthakn7 sarthakn7 merged commit a243ea5 into main Oct 3, 2024
1 check passed
@sarthakn7 sarthakn7 deleted the sarthakn_refactor_handlers branch October 3, 2024 19:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants