add start trial API to HLRC #32799
add start trial API to HLRC #32799
Conversation
|
Pinging @elastic/es-core-infra |
| */ | ||
| public PostStartTrialResponse postStartTrial(PostStartTrialRequest request, RequestOptions options) throws IOException { | ||
| return restHighLevelClient.performRequestAndParseEntity(request, RequestConverters::postStartTrial, options, | ||
| PostStartTrialResponse::fromXContent, singleton(403) |
There was a problem hiding this comment.
If the trial is already started it will return a 403 code but without the exception structure that hlrc is expecting
| assertTrue(latch.await(30L, TimeUnit.SECONDS)); | ||
|
|
||
| // todo add some other cases with randomization | ||
| // todo add a case that succeeds in starting the trial |
There was a problem hiding this comment.
I'm not sure there is an IT test case for this anywhere, although I need to look more thoroughly
There was a problem hiding this comment.
Yea id def add a IT test for it and only keep the DocumentationIT to a minimum, to validate that it actually does what you are documenting.
hub-cap
left a comment
hub-cap
left a comment
There was a problem hiding this comment.
I think u are on the right track. I would love to see you fix the existing xContent representation by moving it into the class first, and then squaring away anything extra you may need for the HLRC. Im not a fan of those custom buildResponse methods in the handlers.
OFC it would be super great if the refactoring / fixing of the xContent and buildResponse was in a separate PR, so it did not clutter this one up.
| Params parameters = new Params(request); | ||
| parameters.withMasterTimeout(postStartTrialRequest.masterNodeTimeout()); | ||
| if (postStartTrialRequest.isAcknowledged()) { | ||
| parameters.putParam("acknowledge", "true"); |
There was a problem hiding this comment.
since this exists in 2 places now, can you extract it into a helper method like the other helpers in this class?
| assertThat(request.getEntity(), nullValue()); | ||
| } | ||
|
|
||
| public void testPostStartTrial() { |
There was a problem hiding this comment.
These typically set an expectedParams map (Map<String, String> expectedParams = new HashMap<>();) so the expected can be validated against the actual in one fell swoop. Also, pls validate the method name/url like other tests do as well.
| boolean trialWasStarted = response.isTrialWasStarted(); | ||
| String errorMessage = response.getErrorMessage(); | ||
| String type = response.getType(); | ||
| String acknowledgeMessage = response.getAcknowledgeMessage(); // todo rename this to acknowledgeheader for consistency |
There was a problem hiding this comment.
will this todo be put in the docs? I dont know the answer, heh. Can you validate it does not get put in the docs?
There was a problem hiding this comment.
The whole line or just the comment? I was going to remove the comment but I'd intended the rest of the line to appear in the docs
There was a problem hiding this comment.
Oh I skimmed that a little too fast, yeah the todo is not intended to remain
| assertTrue(latch.await(30L, TimeUnit.SECONDS)); | ||
|
|
||
| // todo add some other cases with randomization | ||
| // todo add a case that succeeds in starting the trial |
There was a problem hiding this comment.
Yea id def add a IT test for it and only keep the DocumentationIT to a minimum, to validate that it actually does what you are documenting.
| } | ||
| message = parser.text(); | ||
| } else { | ||
| if (token != XContentParser.Token.START_ARRAY) { |
There was a problem hiding this comment.
id prefer to see this in a non nested else {}. its a bit more pleasing to the eye having one less level of nesting here.
if ("message".equals(curr..... {
...
} else if (token == XContentParser.Token.START_ARRAY) {
...
} else {
throw new...
}
There was a problem hiding this comment.
Sure thing, I shamelessly stole this from https://github.com/elastic/elasticsearch/pull/32214/files#diff-a651ec0801020d94344b5d3c51ec39d0R63 so it might be worth sharing between the two
|
Thanks for the review, I'll open another one to consolidate all the response xcontent handling before moving forward here |
|
Hi @andyb-elastic. We decided to split the request and response from the ones used in server/x-pack, so they do not overlap. Please update your PR such that you create new Good news, you dont have to consolidate the request/response anymore! |
|
Closing in favor of #33406 |
4 participants
This is kind of a WIP because I'm not sure about some of the approach I took here, but wanted to get some general feedback
The existing PostStartTrialResponse class has an internal structure that's a little different than its xcontent representation because it stuffs some of its state into a status enum. Although I think I have a better idea now how to handle that inside the class now, so its xcontent generation can probably be moved inside the class. It also is missing the license type field which I think can be added to it here from the request