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

Implementing ApiCall class #4

Closed
happy-san opened this issue Apr 26, 2021 · 12 comments
Closed

Implementing ApiCall class #4

happy-san opened this issue Apr 26, 2021 · 12 comments
Assignees
Labels
enhancement New feature or request

Comments

@happy-san
Copy link
Collaborator

As far as I've understood from gazing at the implementation in JavaScript and Python, here are the requirements summed up:

The main function is the make_request or equivalent. It's responsible for connecting with a node and completing the request. If a nearestNode is present in the Configuration, it'll try to connect to it given it's healthy, if not it'll fall back to the nodes and use them in a round-robin fashion.

I'm not sure how if a node is healthy is tracked? What I think: Every node starts with their health status as "fine" and when eventually if a request fails, that node is marked unhealthy and the next node is tried until eventually due to round-robin this node is considered again, and if the request is successful this time, it's again marked healthy again?
Also, is this where numRetries comes into play? In that, if a node fails to respond, we would try the same node numRetries times?

I'm also not sure about healthcheckInterval present in the Configuration class. What's its use?

@happy-san happy-san added the question Further information is requested label Apr 26, 2021
@jasonbosco
Copy link
Member

What I think: Every node starts with their health status as "fine" and when eventually if a request fails, that node is marked unhealthy and the next node is tried until eventually due to round-robin this node is considered again, and if the request is successful this time, it's again marked healthy again?

Yup, this is exactly how it works.

Also, is this where numRetries comes into play? In that, if a node fails to respond, we would try the same node numRetries times?

No, numRetries defines how many times we'll keep go in the round-robin loop to try and get a healthy node before giving up.

I'm also not sure about healthcheckInterval present in the Configuration class. What's its use?

When a node is marked as unhealthy, healthcheckInterval defines how long an unhealthy node should be kept out of the round-robin list of nodes. This is where it's used in the JS Client:

https://github.com/typesense/typesense-js/blob/e3966a8ed13a3c4f14f84a828fa56f921821bca5/src/Typesense/ApiCall.js#L173-L179

@happy-san
Copy link
Collaborator Author

One more question suppose we have three nodes A, B, and C in the cluster. A is the nearest node. So when creating configuration would A be a part of nodes or not? If not, should have a check regarding this in the initialization of Configuration?

@jasonbosco
Copy link
Member

jasonbosco commented Apr 26, 2021

The nearest node is a 4th node D, that's not part of the nodes array.

The logic is that, if nearestNode D is specified, you'd first always try that first and if it fails, then fallback to round-robinning between nodes A, B & C.

@happy-san happy-san added enhancement New feature or request and removed question Further information is requested labels Apr 26, 2021
@happy-san happy-san assigned happy-san and unassigned jasonbosco Apr 26, 2021
@happy-san
Copy link
Collaborator Author

happy-san commented Apr 27, 2021

@mafreud I'm looking for some suggestions:

I've gone through how the JS and Python clients are making requests. I'm thinking rather than implementing everything ourselves, we could use the http package which will also have the added bonus of giving access to Client to make persistent connections with the respective nodes.

I'm thinking something like this

class ApiCall {
  final Configuration config;
  Map<Node, http.BaseClient> _nodes;
  int _nodeIndex = 0;
  bool _usingNearestNode;
  http.Client _nearestClient;

  ApiCall(this.config) {
    _nodes = Map.fromIterable(
      config.nodes,
      key: (node) => node,
      value: (node) => null, // Lazily initialize the clients.
    );

    _usingNearestNode = config.nearestNode != null;
    if (config.nearestNode != null) {
      _nearestClient = http.Client();
    }
  }
}

but that'll become cumbersome so maybe having a Client in every Node would be a better approach. Then the ApiCall class would use the client of the node that's healthy to complete the request.

Edit:

Also then the ApiCall class can expose the methods like get, post, etc and simply forward the request something like

class ApiCall {
  // ...
  Future<T> post (...) {
    _nodes[_nodeIndex].client.post(...);
  }
}

@mafreud
Copy link
Contributor

mafreud commented Apr 27, 2021

that'll become cumbersome so maybe having a Client in every Node would be a better approach.

@happy-san Depending packages makes projects hard for maintaining, so I agree with you. 😄

@happy-san
Copy link
Collaborator Author

happy-san commented Apr 28, 2021

Oh okay so we don't want to depend on external packages at all? I was saying

but that'll become cumbersome

regarding

​Map​<​Node​, http.​BaseClient​>​ _nodes;

Also I do agree with you that depending on external packages increases the technical debt but I believe since it's a package developed by the dart team itself we can depend on it. What's your take?

@mafreud
Copy link
Contributor

mafreud commented Apr 28, 2021

Right, it's okay to depend http package. it's well maintained by dart team.

@mafreud
Copy link
Contributor

mafreud commented Apr 28, 2021

What I wanted to say is depending unnecessary packages (or too many packages) is not suited for this project. As you probably know!

@happy-san
Copy link
Collaborator Author

I think this'll be the only package we'll depend on.

@happy-san
Copy link
Collaborator Author

happy-san commented Apr 28, 2021

@jasonbosco doing this

this._currentNodeIndex = (this._currentNodeIndex + 1) % this._nodes.length

suggests that we're changing the nodes for every request regardless of whether the node at _currentNodeIndex completed the last request successfully or not. Is it to spread the load to all the nodes?

Also, could you explain the terms endpoint, bodyParameters, queryParameters and additionalHeaders let's say in case we're creating a new collection? From what I understand, endpoint in this case would be /collections, bodyParameters would contain whatever schema is, I'm not sure about the rest of the terms.
I understand all these terms now.

@happy-san
Copy link
Collaborator Author

@jasonbosco I want to understand the use of requestNumber variable. It seems to be used just for logging purposes to me.

@happy-san happy-san mentioned this issue Apr 30, 2021
1 task
@jasonbosco
Copy link
Member

Is it to spread the load to all the nodes?

That’s correct

I want to understand the use of requestNumber variable. It seems to be used just for logging purposes to me.

Correct, it’s just for logging purposes. I found this to be specially helpful when debugging which log line is for which retry attempt.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants