You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@streams.apache.org by steveblackmon <gi...@git.apache.org> on 2015/07/04 19:08:28 UTC
[GitHub] incubator-streams pull request: found and fixed a bug with GraphHt...
GitHub user steveblackmon opened a pull request:
https://github.com/apache/incubator-streams/pull/240
found and fixed a bug with GraphHttpPersistWriter
introduced with first STREAMS-313 PR, added test case
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/steveblackmon/incubator-streams STREAMS-313.2
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-streams/pull/240.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #240
----
commit bc18b6e145bc586888b35914b4c6b515e145405b
Author: Steve Blackmon (@steveblackmon) <sb...@apache.org>
Date: 2015-07-04T17:07:55Z
found and fixed a bug with GraphHttpPersistWriter introduced with first STREAMS-313 PR, added test case
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-streams pull request: found and fixed a bug with GraphHt...
Posted by steveblackmon <gi...@git.apache.org>.
Github user steveblackmon commented on the pull request:
https://github.com/apache/incubator-streams/pull/240#issuecomment-126482766
@eponvert I agree, updating per your comment regarding entry.getDocument()
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-streams pull request: found and fixed a bug with GraphHt...
Posted by steveblackmon <gi...@git.apache.org>.
Github user steveblackmon commented on a diff in the pull request:
https://github.com/apache/incubator-streams/pull/240#discussion_r35919668
--- Diff: streams-contrib/streams-persist-graph/src/main/java/org/apache/streams/graph/GraphHttpPersistWriter.java ---
@@ -84,54 +86,79 @@ else if( configuration.getType().equals(GraphHttpConfiguration.Type.REXSTER)) {
protected ObjectNode preparePayload(StreamsDatum entry) {
Activity activity = null;
+ ActivityObject activityObject = null;
if (entry.getDocument() instanceof Activity) {
activity = (Activity) entry.getDocument();
- } else if (entry.getDocument() instanceof ObjectNode) {
- activity = mapper.convertValue(entry.getDocument(), Activity.class);
- } else if (entry.getDocument() instanceof String) {
- try {
- activity = mapper.readValue((String) entry.getDocument(), Activity.class);
- } catch (Throwable e) {
- LOGGER.warn(e.getMessage());
+ } else if (entry.getDocument() instanceof ActivityObject) {
+ activityObject = (ActivityObject) entry.getDocument();
+ } else {
+ ObjectNode objectNode;
+ if (entry.getDocument() instanceof ObjectNode) {
+ objectNode = (ObjectNode) entry.getDocument();
+ } else if( entry.getDocument() instanceof String) {
+ try {
+ objectNode = mapper.readValue((String) entry.getDocument(), ObjectNode.class);
+ } catch (IOException e) {
+ LOGGER.error("Can't handle input: ", entry);
+ return mapper.createObjectNode();
+ }
+ } else {
+ LOGGER.error("Can't handle input: ", entry);
+ return mapper.createObjectNode();
--- End diff --
I can see your point. I'll change the interface to allow exceptions to be thrown, with the default response in the parent class being to use a blank payload.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-streams pull request: found and fixed a bug with GraphHt...
Posted by eponvert <gi...@git.apache.org>.
Github user eponvert commented on a diff in the pull request:
https://github.com/apache/incubator-streams/pull/240#discussion_r33950035
--- Diff: streams-contrib/streams-persist-graph/src/main/java/org/apache/streams/graph/GraphHttpPersistWriter.java ---
@@ -84,54 +86,79 @@ else if( configuration.getType().equals(GraphHttpConfiguration.Type.REXSTER)) {
protected ObjectNode preparePayload(StreamsDatum entry) {
Activity activity = null;
+ ActivityObject activityObject = null;
if (entry.getDocument() instanceof Activity) {
activity = (Activity) entry.getDocument();
- } else if (entry.getDocument() instanceof ObjectNode) {
- activity = mapper.convertValue(entry.getDocument(), Activity.class);
- } else if (entry.getDocument() instanceof String) {
- try {
- activity = mapper.readValue((String) entry.getDocument(), Activity.class);
- } catch (Throwable e) {
- LOGGER.warn(e.getMessage());
+ } else if (entry.getDocument() instanceof ActivityObject) {
+ activityObject = (ActivityObject) entry.getDocument();
+ } else {
+ ObjectNode objectNode;
+ if (entry.getDocument() instanceof ObjectNode) {
+ objectNode = (ObjectNode) entry.getDocument();
+ } else if( entry.getDocument() instanceof String) {
+ try {
+ objectNode = mapper.readValue((String) entry.getDocument(), ObjectNode.class);
+ } catch (IOException e) {
+ LOGGER.error("Can't handle input: ", entry);
+ return mapper.createObjectNode();
--- End diff --
Is eating the exception and returning a default object node the right strategy here? Shouldn't the IOE be thrown up to the caller?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-streams pull request: found and fixed a bug with GraphHt...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-streams/pull/240
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-streams pull request: found and fixed a bug with GraphHt...
Posted by steveblackmon <gi...@git.apache.org>.
Github user steveblackmon commented on a diff in the pull request:
https://github.com/apache/incubator-streams/pull/240#discussion_r35919656
--- Diff: streams-contrib/streams-persist-graph/src/main/java/org/apache/streams/graph/GraphHttpPersistWriter.java ---
@@ -84,54 +86,79 @@ else if( configuration.getType().equals(GraphHttpConfiguration.Type.REXSTER)) {
protected ObjectNode preparePayload(StreamsDatum entry) {
Activity activity = null;
+ ActivityObject activityObject = null;
if (entry.getDocument() instanceof Activity) {
activity = (Activity) entry.getDocument();
- } else if (entry.getDocument() instanceof ObjectNode) {
- activity = mapper.convertValue(entry.getDocument(), Activity.class);
- } else if (entry.getDocument() instanceof String) {
- try {
- activity = mapper.readValue((String) entry.getDocument(), Activity.class);
- } catch (Throwable e) {
- LOGGER.warn(e.getMessage());
+ } else if (entry.getDocument() instanceof ActivityObject) {
+ activityObject = (ActivityObject) entry.getDocument();
+ } else {
+ ObjectNode objectNode;
+ if (entry.getDocument() instanceof ObjectNode) {
+ objectNode = (ObjectNode) entry.getDocument();
+ } else if( entry.getDocument() instanceof String) {
+ try {
+ objectNode = mapper.readValue((String) entry.getDocument(), ObjectNode.class);
+ } catch (IOException e) {
+ LOGGER.error("Can't handle input: ", entry);
+ return mapper.createObjectNode();
--- End diff --
I can see your point. I'll change the interface to allow exceptions to be thrown, with the default response in the parent class being to use a blank payload.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-streams pull request: found and fixed a bug with GraphHt...
Posted by eponvert <gi...@git.apache.org>.
Github user eponvert commented on a diff in the pull request:
https://github.com/apache/incubator-streams/pull/240#discussion_r33950499
--- Diff: streams-contrib/streams-persist-graph/src/main/java/org/apache/streams/graph/GraphHttpPersistWriter.java ---
@@ -84,54 +86,79 @@ else if( configuration.getType().equals(GraphHttpConfiguration.Type.REXSTER)) {
protected ObjectNode preparePayload(StreamsDatum entry) {
Activity activity = null;
+ ActivityObject activityObject = null;
if (entry.getDocument() instanceof Activity) {
activity = (Activity) entry.getDocument();
- } else if (entry.getDocument() instanceof ObjectNode) {
- activity = mapper.convertValue(entry.getDocument(), Activity.class);
- } else if (entry.getDocument() instanceof String) {
- try {
- activity = mapper.readValue((String) entry.getDocument(), Activity.class);
- } catch (Throwable e) {
- LOGGER.warn(e.getMessage());
+ } else if (entry.getDocument() instanceof ActivityObject) {
+ activityObject = (ActivityObject) entry.getDocument();
+ } else {
+ ObjectNode objectNode;
+ if (entry.getDocument() instanceof ObjectNode) {
+ objectNode = (ObjectNode) entry.getDocument();
+ } else if( entry.getDocument() instanceof String) {
+ try {
+ objectNode = mapper.readValue((String) entry.getDocument(), ObjectNode.class);
+ } catch (IOException e) {
+ LOGGER.error("Can't handle input: ", entry);
+ return mapper.createObjectNode();
+ }
+ } else {
+ LOGGER.error("Can't handle input: ", entry);
+ return mapper.createObjectNode();
+ }
+
+ if( objectNode.get("verb") != null ) {
+ try {
+ activity = mapper.convertValue(objectNode, Activity.class);
+ } catch (Exception e) {
+ activityObject = mapper.convertValue(objectNode, ActivityObject.class);
+ }
+ } else {
+ activityObject = mapper.convertValue(objectNode, ActivityObject.class);
}
}
- Preconditions.checkNotNull(activity);
+ Preconditions.checkArgument(activity != null || activityObject != null);
ObjectNode request = mapper.createObjectNode();
ArrayNode statements = mapper.createArrayNode();
- activity.getActor().setObjectType("page");
-
// always add vertices first
- List<String> labels = Lists.newArrayList();
- if( activity.getProvider() != null &&
- !Strings.isNullOrEmpty(activity.getProvider().getId()) ) {
- labels.add(activity.getProvider().getId());
- }
+ List<String> labels = Lists.newArrayList("streams");
- if( activity.getActor() != null &&
- !Strings.isNullOrEmpty(activity.getActor().getId()) ) {
- if( activity.getActor().getObjectType() != null )
- labels.add(activity.getActor().getObjectType());
- statements.add(httpGraphHelper.createHttpRequest(queryGraphHelper.mergeVertexRequest(activity.getActor())));
+ if( activityObject != null ) {
+ if (activityObject.getObjectType() != null)
+ labels.add(activityObject.getObjectType());
+ statements.add(httpGraphHelper.createHttpRequest(queryGraphHelper.mergeVertexRequest(activityObject)));
}
- if( activity.getObject() != null &&
- !Strings.isNullOrEmpty(activity.getObject().getId()) ) {
- if( activity.getObject().getObjectType() != null )
- labels.add(activity.getObject().getObjectType());
- statements.add(httpGraphHelper.createHttpRequest(queryGraphHelper.mergeVertexRequest(activity.getObject())));
- }
+ if( activity != null ) {
+ if( activity.getProvider() != null &&
--- End diff --
More style points, similar comment as before, basically -- I'd save `provider`, `actor`, `object` variables here, I think would be more readable than calling `activity.getProvider()` etc in what follows.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-streams pull request: found and fixed a bug with GraphHt...
Posted by steveblackmon <gi...@git.apache.org>.
Github user steveblackmon commented on a diff in the pull request:
https://github.com/apache/incubator-streams/pull/240#discussion_r35918901
--- Diff: streams-contrib/streams-persist-graph/src/main/java/org/apache/streams/graph/GraphHttpPersistWriter.java ---
@@ -84,54 +86,79 @@ else if( configuration.getType().equals(GraphHttpConfiguration.Type.REXSTER)) {
protected ObjectNode preparePayload(StreamsDatum entry) {
Activity activity = null;
+ ActivityObject activityObject = null;
if (entry.getDocument() instanceof Activity) {
activity = (Activity) entry.getDocument();
- } else if (entry.getDocument() instanceof ObjectNode) {
- activity = mapper.convertValue(entry.getDocument(), Activity.class);
- } else if (entry.getDocument() instanceof String) {
- try {
- activity = mapper.readValue((String) entry.getDocument(), Activity.class);
- } catch (Throwable e) {
- LOGGER.warn(e.getMessage());
+ } else if (entry.getDocument() instanceof ActivityObject) {
+ activityObject = (ActivityObject) entry.getDocument();
+ } else {
+ ObjectNode objectNode;
+ if (entry.getDocument() instanceof ObjectNode) {
+ objectNode = (ObjectNode) entry.getDocument();
+ } else if( entry.getDocument() instanceof String) {
+ try {
+ objectNode = mapper.readValue((String) entry.getDocument(), ObjectNode.class);
+ } catch (IOException e) {
+ LOGGER.error("Can't handle input: ", entry);
+ return mapper.createObjectNode();
+ }
+ } else {
+ LOGGER.error("Can't handle input: ", entry);
+ return mapper.createObjectNode();
+ }
+
+ if( objectNode.get("verb") != null ) {
+ try {
+ activity = mapper.convertValue(objectNode, Activity.class);
+ } catch (Exception e) {
+ activityObject = mapper.convertValue(objectNode, ActivityObject.class);
+ }
+ } else {
+ activityObject = mapper.convertValue(objectNode, ActivityObject.class);
}
}
- Preconditions.checkNotNull(activity);
+ Preconditions.checkArgument(activity != null || activityObject != null);
ObjectNode request = mapper.createObjectNode();
ArrayNode statements = mapper.createArrayNode();
- activity.getActor().setObjectType("page");
-
// always add vertices first
- List<String> labels = Lists.newArrayList();
- if( activity.getProvider() != null &&
- !Strings.isNullOrEmpty(activity.getProvider().getId()) ) {
- labels.add(activity.getProvider().getId());
- }
+ List<String> labels = Lists.newArrayList("streams");
- if( activity.getActor() != null &&
- !Strings.isNullOrEmpty(activity.getActor().getId()) ) {
- if( activity.getActor().getObjectType() != null )
- labels.add(activity.getActor().getObjectType());
- statements.add(httpGraphHelper.createHttpRequest(queryGraphHelper.mergeVertexRequest(activity.getActor())));
+ if( activityObject != null ) {
+ if (activityObject.getObjectType() != null)
+ labels.add(activityObject.getObjectType());
+ statements.add(httpGraphHelper.createHttpRequest(queryGraphHelper.mergeVertexRequest(activityObject)));
}
- if( activity.getObject() != null &&
- !Strings.isNullOrEmpty(activity.getObject().getId()) ) {
- if( activity.getObject().getObjectType() != null )
- labels.add(activity.getObject().getObjectType());
- statements.add(httpGraphHelper.createHttpRequest(queryGraphHelper.mergeVertexRequest(activity.getObject())));
- }
+ if( activity != null ) {
+ if( activity.getProvider() != null &&
--- End diff --
I agree, updating
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-streams pull request: found and fixed a bug with GraphHt...
Posted by eponvert <gi...@git.apache.org>.
Github user eponvert commented on the pull request:
https://github.com/apache/incubator-streams/pull/240#issuecomment-118907988
Style-points/personal preference: instead of calling
```java
entry.getDocument()
```
so many times, I'd put that in a variable up front.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-streams pull request: found and fixed a bug with GraphHt...
Posted by eponvert <gi...@git.apache.org>.
Github user eponvert commented on the pull request:
https://github.com/apache/incubator-streams/pull/240#issuecomment-118908880
GraphHttpPersistWriter.java is now inconsistent with white-space conventions -- I'm seeing both
```
if( condition ) {
```
and
```
if (condition) {
```
I prefer the latter, though I think the former is more prevalent in the streams code I've looked at; anyway, these should probably be consistent.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-streams pull request: found and fixed a bug with GraphHt...
Posted by eponvert <gi...@git.apache.org>.
Github user eponvert commented on a diff in the pull request:
https://github.com/apache/incubator-streams/pull/240#discussion_r33950138
--- Diff: streams-contrib/streams-persist-graph/src/main/java/org/apache/streams/graph/GraphHttpPersistWriter.java ---
@@ -84,54 +86,79 @@ else if( configuration.getType().equals(GraphHttpConfiguration.Type.REXSTER)) {
protected ObjectNode preparePayload(StreamsDatum entry) {
Activity activity = null;
+ ActivityObject activityObject = null;
if (entry.getDocument() instanceof Activity) {
activity = (Activity) entry.getDocument();
- } else if (entry.getDocument() instanceof ObjectNode) {
- activity = mapper.convertValue(entry.getDocument(), Activity.class);
- } else if (entry.getDocument() instanceof String) {
- try {
- activity = mapper.readValue((String) entry.getDocument(), Activity.class);
- } catch (Throwable e) {
- LOGGER.warn(e.getMessage());
+ } else if (entry.getDocument() instanceof ActivityObject) {
+ activityObject = (ActivityObject) entry.getDocument();
+ } else {
+ ObjectNode objectNode;
+ if (entry.getDocument() instanceof ObjectNode) {
+ objectNode = (ObjectNode) entry.getDocument();
+ } else if( entry.getDocument() instanceof String) {
+ try {
+ objectNode = mapper.readValue((String) entry.getDocument(), ObjectNode.class);
+ } catch (IOException e) {
+ LOGGER.error("Can't handle input: ", entry);
+ return mapper.createObjectNode();
+ }
+ } else {
+ LOGGER.error("Can't handle input: ", entry);
+ return mapper.createObjectNode();
--- End diff --
Same question, shouldn't an exception be thrown here?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---