-
Notifications
You must be signed in to change notification settings - Fork 53
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
eureka 1.3.1 failed parse JSON register #29
Comments
Hrm, so I've started working on this, but it turns out there's more to it than just the DataCenterInfo class. It looks like there are some inconsistencies with how Ports are represented introduced in 1.3.1. I see, for example, some ports as Also, it looks like they require incoming ports in JSON to be represented as integers, which is incompatible with the 1.1.X series of APIs. Given this, I'm thinking about dropping the JSON API altogether and using only XML since it seems to be the "first class" way to interact with Eureka. It shouldn't need to affect the public API since the UseJSON What do the rest of the contributors think about that? @tysonstewart @Camtendo @jcox92 |
I have to use JSON API, since the InstanceMetadata with XML API seems not to work. thx in adv. |
@ryansb That seems reasonable to me. I'm not at all familiar with the differences (if any) between the JSON and XML APIs so I'll defer to your judgement on that. All of our internal tools that use Fargo only use the XML API so we should be good on that front! |
I haven't contributed actual code to this (just the OSS badges) so I'll also defer my opinion. That proposition makes sense to me though. |
@jcox92 what version of Eureka are you going to be using going forward? Will you be migrating to 1.3.x, or will you stay on the 1.1.x series? |
@ryansb We're planning on staying on 1.1.x going forward. As of right now we don't have any plans to upgrade that I'm aware of. |
I'm using Eureka 1.3 Any chance you would consider modifying fargo.Instance so that we can pass in our own JSON, and it is just stored as json, and then that json is passed to Eureka during registration? That would put the burden on the user if they choose/need to use JSON, but you wouldn't have to keep modifying your code. By the way, I can't get the xml to work either for Eureka 1.3, so this issue really unfortunately means I can't use your awesome package. |
I stand corrected XML does work with Eureka 1.3 I've included the XML output, in case anyone else is having difficulty. 2016/09/29 15:47:12 Registering instance with url http://localhost:8761/eureka/apps/ES-Search |
What isn't working correctly with the JSON that fargo is sending to Eureka? Is it the same problem noted above? |
Yes, it is the same problem as noted above - there isn't a way to add the "@ class": "com.netflix.appinfo.MyDataCenterInfo" to the Instance struct. Here was the output 2016/09/29 15:03:24 Registering instance with url http://localhost:8761/eureka/apps/ES-Search When I manually POSTed via poster the same json but with the "@ class": "com.netflix.appinfo.MyDataCenterInfo" added to the DataCenterInfo block, it worked. |
That's odd; the Eureka server doesn't demand that that field be present for the XML document. After I've registered an instance and I ask Eureka to send it back, that field is present—populated by Eureka on its own. I take it the proposal is that we send that field when registering an instance. Before doing that, I'd like to study the Eureka server code to see why it's complaining about this lack. |
Eureka release 1.2.2 introduced the Jackson codec, which brings along encoding the ports as integers. I was about to go try to handle this when I read @ryansb's earlier comment about the rest of the changes. What a mess. |
As I've been working on resolving this problem, here's what I've learned:
|
See pull request #59 for a proposed solution to this problem. |
With eureka 1.3.1, JSON doesn't work anymore. It is rejected:
HTTP returned 400 registering Instance ... Body="{"error": "cannot parse request body"}"
The problem seems come from DataCenterInfo where:
The text was updated successfully, but these errors were encountered: