Skip to content
This repository has been archived by the owner on Jul 27, 2023. It is now read-only.

Missing fields for session creation #66

Closed
JensRantil opened this issue Sep 21, 2015 · 20 comments
Closed

Missing fields for session creation #66

JensRantil opened this issue Sep 21, 2015 · 20 comments

Comments

@JensRantil
Copy link

The Consul documentation on sessions states that one can specify the following fields when creating a session:

{
  "LockDelay": "15s",
  "Name": "my-service-lock",
  "Node": "foobar",
  "Checks": ["a", "b", "c"],
  "Behavior": "release",
  "TTL": "0s"
}

Based on the methods on the SessionClient, it looks like only Name (parameter name value I assume is name) can be given. Let me know if I've missed something.

@rickfast
Copy link
Owner

For some reason, the session client takes JSON as a string. I haven't fixed it yet, and I wasn't very diligent when I reviewed the session client pull request. I can fix.

@JensRantil
Copy link
Author

@rickfast Awesome. 👍

@defucius
Copy link

Is there any progress on this addition? I also ran into an error when trying to read the session info by calling getSessionInfo(sessionId). The error seems to say the TTL value of "30s" cannot be demarshaled to a String?

Caused by: com.fasterxml.jackson.databind.JsonMappingException: Instantiation of [simple type, class com.orbitz.consul.model.session.ImmutableSessionInfo] value failed: argument type mismatch (through reference chain: Object[][0]->com.orbitz.consul.model.session.ImmutableSessionInfo["TTL"])

command line result:
curl http://localhost:8500/v1/session/info/0de22989-9ad4-c5e5-b816-b2fdccde4336
[{"CreateIndex":13491,"ID":"0de22989-9ad4-c5e5-b816-b2fdccde4336","Name":"","Node":"agent-pro","Checks":["serfHealth"],"LockDelay":15000000000,"Behavior":"release","TTL":"30s"}]

@rickfast
Copy link
Owner

Orbitz has been in the process of integration with Expedia the past two weeks, and I've been mostly unable to code. I should be able to tackle this early next week. Sorry for the slowness. As always, PRs welcome.

@gjesse
Copy link
Contributor

gjesse commented Oct 19, 2015

@rickfast , I was going to take a look at #70, but after checking out the existing code + this issue, it seems the lib would be better served by rewriting the session client. I may be able to put in some time on it this week, but I don't want to duplicate efforts as it's non-trivial. Are you still planning to work on this? want me to take a crack at it?

@rickfast
Copy link
Owner

I'm actually rewriting it as we speak. I wish I had looked harder at the pull request with this. Should be ready in just a bit.

@gjesse
Copy link
Contributor

gjesse commented Oct 19, 2015

nice! i'll be happy to review it if u like

@rickfast
Copy link
Owner

Please: #71

@rickfast
Copy link
Owner

Fixed by edfdd46

@JensRantil
Copy link
Author

Awesome! Do you think you could maybe make a release with this? 🙏 ;)

@rickfast
Copy link
Owner

Yup. I'll do it first thing this morning

@JensRantil
Copy link
Author

❤️

@rickfast
Copy link
Owner

0.9.11 is available in jcenter and central

@JensRantil
Copy link
Author

Sweet! Thanks!

@defucius
Copy link

Have anyone tried to set the session parameters with the new implementation? It seems that creating a session would fail with a 400 Bad Request if the ttl or lockDelay is set. Creating an empty session or a session with name as a single parameter works as in the test class. I haven’t tried the other three parameters.

This is with Maven build, ConsulAgent 0.9.11. My code looks like this:

Session sessionSettings = ImmutableSession.builder().lockDelay("30").build();
String sessionId = sessionClient.createSession(sessionSettings).getId();

The stack trace:

Exception in thread "main" javax.ws.rs.BadRequestException: HTTP 400 Bad Request
at org.glassfish.jersey.client.JerseyInvocation.convertToException(JerseyInvocation.java:884)
at org.glassfish.jersey.client.JerseyInvocation.translate(JerseyInvocation.java:802)
at org.glassfish.jersey.client.JerseyInvocation.access$600(JerseyInvocation.java:89)
at org.glassfish.jersey.client.JerseyInvocation$3.call(JerseyInvocation.java:673)
at org.glassfish.jersey.internal.Errors.process(Errors.java:315)
at org.glassfish.jersey.internal.Errors.process(Errors.java:297)
at org.glassfish.jersey.internal.Errors.process(Errors.java:228)
at org.glassfish.jersey.process.internal.RequestScope.runInScope(RequestScope.java:421)
at org.glassfish.jersey.client.JerseyInvocation.invoke(JerseyInvocation.java:669)
at org.glassfish.jersey.client.JerseyInvocation$Builder.method(JerseyInvocation.java:414)
at org.glassfish.jersey.client.JerseyInvocation$Builder.put(JerseyInvocation.java:296)
at com.orbitz.consul.SessionClient.createSession(SessionClient.java:71)
at com.orbitz.consul.SessionClient.createSession(SessionClient.java:52)

Did I miss anything in my code?

On Oct 24, 2015, at 11:24 AM, Jens Rantil notifications@github.com wrote:

Sweet! Thanks!


Reply to this email directly or view it on GitHub #66 (comment).

@rickfast
Copy link
Owner

Try "30s". Either way, I will add better tests and fix any issues. Thanks!

@rickfast rickfast reopened this Oct 29, 2015
@defucius
Copy link

“30s” works. Thanks!

On Oct 29, 2015, at 9:25 AM, Rick Fast notifications@github.com wrote:

Try "30s". Either way, I will add better tests and fix any issues. Thanks!


Reply to this email directly or view it on GitHub #66 (comment).

@JensRantil
Copy link
Author

@rickfast I assume Consul returns some kind of error message on that 400. Would it make sense to try to parse the 400 error page and include it in the exception? Would make debugging errors like @defucius's much easier. Just an idea...

@rickfast
Copy link
Owner

rickfast commented Nov 4, 2015

Yeah, I can add this

@JensRantil
Copy link
Author

👍

@rickfast rickfast closed this as completed Jan 6, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants