search

tango-controls / JTango

TANGO kernel Java implementation. JTango moved to https://gitlab.com/tango-controls/JTango
http://tango-controls.org
8 stars 14 forks source link

Attempt to fix random CORBA timeouts #98

Closed Ingvord closed 4 years ago

Ingvord commented 4 years ago

Resolves #19 (?)

Ingvord commented 4 years ago

Is getClientHostName() now returning an IP address instead of an hostname?

Yes. I would say if client needs hostName it should do name resolving

bourtemb commented 4 years ago

Is getClientHostName() now returning an IP address instead of an hostname?

Yes. I would say if client needs hostName it should do name resolving

I think it is expected for a method named getClientHostName() to return a hostname, not an IP address. This is why the method was doing the name resolving. As far as I see, in JTango code, this seems to be used at least in the device blackbox, in InvocationContext.getClientHostName() and in DeviceManager.getClientHostName(). I have no clear idea about the impact on our users. One obvious impact is that they will see the IP address of the client instead of its hostname in the blackbox results, which is not very nice for the users compared to what they are used to. I don't know where InvocationContext.getClientHostName() and DeviceManager.getClientHostName() are used. At first sight it looks like this pull request is downgrading in some way the user experience even though it is trying to solve something which would also improve the user experience, for the users having troubles configuring their network. I'd be happy to hear the opinion of other users...

Ingvord commented 4 years ago

@bourtemb , I totally agree with you about the method name etc.

But we (HZG, DESY) are users as well and our experience currently with random timeouts rather poor. I would sacrifice fancy host name in the blackbox (which we do not use BTW) in favor of not having random timeouts due to the fact that Tango makes undocumented hidden call to DNS on every client request.

Of course, I agree with you to wait for others input

codecov[bot] commented 4 years ago

Codecov Report

Merging #98 into jtango-9-lts will decrease coverage by 0.03%. The diff coverage is 100%.

Impacted file tree graph

@@                Coverage Diff                 @@
##             jtango-9-lts      #98      +/-   ##
==================================================
- Coverage           25.59%   25.56%   -0.04%     
  Complexity           1545     1545              
==================================================
  Files                 312      312              
  Lines               21248    21244       -4     
  Branches             2391     2391              
==================================================
- Hits                 5438     5430       -8     
- Misses              15342    15346       +4     
  Partials              468      468
Impacted Files Coverage Δ Complexity Δ
...n/java/org/tango/orb/ServerRequestInterceptor.java 63.63% <100%> (-1.23%) 12 <1> (ø)
.../src/main/java/org/tango/server/ServerManager.java 56.41% <0%> (-3.21%) 21% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 14a6527...db824b8. Read the comment docs.

bourtemb commented 4 years ago

As @taurel suggested to me , the right approach would be to do like in cppTango: to do the host naming resolution only when the blackbox is requested, and not every time a CORBA request is received as it seems to be the case in the current version (but maybe I didn't interpret well the current code). I think this solution would make everybody happy.

bourtemb commented 4 years ago

Still I am not sure about the side effects on InvocationContext and DeviceManager classes which are also exposing a getClientHostName() method which seems to use ServerRequestInterceptor.getClientHostName() I have no idea whether anyone is using it and in which context.

Ingvord commented 4 years ago

Sure, @gwen-soleil should have more insights.

gwen-soleil commented 4 years ago

Maybe the best solution is to add a feature toggle (with a Java system property and/or a setter in ServerManager)?

Ingvord commented 4 years ago

@gwen-soleil , thanks for your input

these method is deprecated since a while now, maybe we should remove them? I don't think it has been really used.

Perfect let's remove them :) Less code to support -> less error prone

So if we move the name resolution to InvocationContext, it can affect also the performance of the device (in case of using @AroundInvoke).

Well in this case we can document - "Do not use unless absolutely necessary". But I would prefer to remove this method at all and just provide ip in InvocationContext. If required client code will be forced to do name resolving and it will be explicit:

@AroundInvoke
public void aroundInvoke(InvocationContext ctx){
  String hostName = InetAddress.getByName(ctx.getClientIPAddress()).getCanonicalHostName();
  //...
}

Maybe the best solution is to add a feature toggle (with a Java system property and/or a setter in ServerManager)?

I strongly disagree - all these toggles... very quickly become an undocumented mess with tons of if-else statements in the code. As it is not a really wide spread feature right now the best is to push it as far away from the core library as possible i.e. into the end-client code

bourtemb commented 4 years ago 
* For https://github.com/tango-controls/JTango/blob/14a6527e3ca67339e51a905ef6c55d773ed04cb7/server/src/main/java/org/tango/server/device/DeviceManager.java#L341-L344
  : these method is deprecated since a while now, maybe we should remove them? I don't think it has been really used.

Good!

* InvocationContext it used to pass the context to the equivalent of "always_executed_hook" in cppTango (with Java annotation AroundInvoke) . So if we move the name resolution to InvocationContext, it can affect also the performance of the device (in case of using @AroundInvoke).

So does this mean that some device servers may rely on that to get the client hostname information obtained from the InvocationContext? Well, if we do the name resolution in InvocationContext.getClientHostName(), the name resolution will occur only if the device server asks for it? So it will affect the performances of the device servers requiring it, which sounds fine to me.

The solution proposed by @ingvord to provide only the IP address from InvocationContext might be good enough... unless there are really many device servers relying on that feature?

Maybe the best solution is to add a feature toggle (with a Java system property and/or a setter in ServerManager)?

I am not convinced it is a good idea to do an host name resolution at each CORBA request in any case. At the ESRF, we really enjoy having the hostnames in the blackbox results and we would like to keep this feature at least. But in this case, the name resolution can be done when creating the DevVarStringArray reply to the BlackBox command.

Ingvord commented 4 years ago

So summing up:

1) remove getClientHostName from both DeviceManager and InvocationContext. Device servers that use them won't compile - forcing users to rewrite the logic. This must be clearly indicated in the release notes

2) In black box do name resolving on demand

If agreed let s proceed

gwen-soleil commented 4 years ago

Hi, I found another solution that I think can comply with both needs. I have done the name resolution asynchronously, cf commit 43561ceaa397e414f589a05e30fc0fe2a8fd0eff