sapia-oss / corus

Corus server
http://www.sapia-oss.org/projects/corus/index.html
GNU General Public License v3.0
2 stars 1 forks source link

Need a way to perform local diagnostic #40

Closed jogoussard closed 7 years ago

jogoussard commented 7 years ago

The diagnostic API doesn't allow to get a local diagnostic in an IP independent way. Assuming you want to compute the health of each of your corus instances from a load balancer, you want to use a generic URL that is valid for each of the nodes. Currently the only way to do this is to use /rest/clusters/{clustername}/hosts/{host}/diagnostic i.e. the LB needs to use a different URL for each host. This cannot be done inAWS for example - ELB healthchecks do not support templated URLs. The simplest change would be to accept /rest/clusters/{clustername}/hosts/localhost:33000/diagnostic

jogoussard commented 7 years ago

I implemented the feature locally simply adding


  // --------------------------------------------------------------------------

  @Path({
      "/clusters/{corus:cluster}/hosts/localhost/diagnostic"
  })
  @HttpMethod(HttpMethod.GET)
  @Output(ContentTypes.APPLICATION_JSON)
  @Accepts({ContentTypes.APPLICATION_JSON, ContentTypes.ANY})
  public String getDiagnosticsForLocalHost(RequestContext context, RestResponseFacade responseFacade) {
    TCPAddress localAddress = context.getConnector().getContext().getServerHost().getEndpoint().getServerTcpAddress();
    ClusterInfo cluster = ClusterInfo.fromLiteralForm(localAddress.getHost()+":"+localAddress.getPort());
    return doGetDiagnostics(context, cluster, responseFacade);
  }

to DiagnosticResource. On a side note, it seems that the clustername in the REST request path is completely ignored by the diagnostic API - I guess that's here for future use?

jcdesrochers commented 7 years ago

Seems quite good! Wondering if we should not add this 'localhost' target on every api call...

yduchesne commented 7 years ago

1) About the cluster name in the REST request path: exactly, that's for future use. This would allow implementing the API separately, in the context of a centralized controller that would have the ability to "talk" to different clusters. In an intra-cluster scenario, the cluster name isn't used because a node in a cluster is not meant to talk to a node in another.

2) Thanks for the new feature. If that does the job let's do it like that for now. Like JC says, it would be a good thing to make it more generic: I'll make it so that we can pass in localhost for {host} in every call. But for now let's do it your way: just submit a PR when you're ready.

jogoussard commented 7 years ago

I would have submitted a PR but my branch push got rejected with 403 - do you need to add me as contributor?

yduchesne commented 7 years ago

Looks like it: I've added you as a contributor... I'll see if there's a way to make this the default for anyone. You should be able to push now.