Skip to content
This repository has been archived by the owner on Nov 15, 2019. It is now read-only.

[WIP] QUARKS-96: postJson in HttpStreams #106

Merged
merged 10 commits into from Jun 3, 2016

Conversation

saurabhjinturkar
Copy link
Contributor

No description provided.

@@ -46,6 +47,12 @@ Licensed to the Apache Software Foundation (ASF) under one
t -> HttpGet.METHOD_NAME, uri, HttpResponders.json());
}

public static TStream<JsonObject> postJson(TStream<JsonObject> stream,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add sufficient javadoc. extra credit: see https://issues.apache.org/jira/browse/QUARKS-97 :-)

@saurabhjinturkar
Copy link
Contributor Author

Hi @dlaboss,

I have added functionality to support postJson() method. I added a method to add body to HTTP requests. Could you please review the code? I will fix review comments and failing build together.

* request1.addProperty("a", "abc");
* request1.addProperty("b", "42");
* TStream<JsonObject> rc = HttpStreams.getJson(
* topology.collection(Arrays.asList(request1)),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: move the construction of the input stream outside of the getJson() invocation? It's not something that will ever occur in practice. Ditto the postJson() example below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please elaborate on this one? I am not clear on this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e.g.,

TStream<JsonObject> requests = topology.collection(Arrays.asList(request1));
TStream<JsonObject> rc = HttpStreams.getJson(requests, ...)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh okay.. i was thinking something complex and weird.

t -> HttpGet.METHOD_NAME, uri, HttpResponders.json());
}

public static TStream<JsonObject> deleteJson(TStream<JsonObject> stream,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

javadoc for {delete,put}Json()?

@saurabhjinturkar
Copy link
Contributor Author

Hi @dlaboss,
Regarding build failure, it seems there is some issue with SSL and certs. I checked logs for failed build of #116. I found that its the same reason as for #106. Could you please guide on this?

*
* TStream<JsonObject> stream = topology.collection(Arrays.asList(body));
* TStream<JsonObject> rc = HttpStreams.postJson(stream,
* HttpClients::noAuthentication, t -> url, t -> body);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you really want/mean t -> body. Presumably the sample is trying to demonstrate posting each tuple from a stream as the body of a POST to the specified url. To do that you need to specify 't -> t' (or Functions.identity()), right?

Also the description of @param body needs work. The function isn't returning an HttpEntity anymore. It's a function that for each JsonObject tuple from stream, returns a JsonObject whose JSON will be the body of the POST request. Right?

@dlaboss
Copy link
Member

dlaboss commented May 18, 2016

sorry for the delay...
I've created pr #119 to remove these tests that aren't robust in the travis/ci environment.
Hopefully the build tests will run clean after your next commits to address the latest comments and I can then merge this PR. Thanks!

@saurabhjinturkar
Copy link
Contributor Author

Hi @dlaboss, It's final exam week for me. Sorry for the delay in fixing review comments.

@dlaboss
Copy link
Member

dlaboss commented May 26, 2016

no worries. hope things go well for you!

@asfgit asfgit merged commit 9212be3 into apache:master Jun 3, 2016
asfgit pushed a commit that referenced this pull request Jun 3, 2016
@dlaboss
Copy link
Member

dlaboss commented Jun 3, 2016

Thanks for your contribution!

@saurabhjinturkar
Copy link
Contributor Author

Thank you for your guidance! :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants