You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tinkerpop.apache.org by rjbriody <gi...@git.apache.org> on 2016/02/24 21:12:15 UTC
[GitHub] incubator-tinkerpop pull request: TINKERPOP-958: split .profile() ...
GitHub user rjbriody opened a pull request:
https://github.com/apache/incubator-tinkerpop/pull/242
TINKERPOP-958: split .profile() into ProfileSideEffectStep and ProfileStep (which is a MapStep).
https://issues.apache.org/jira/browse/TINKERPOP-958
I split .profile() into ProfileSideEffectStep and ProfileStep (which is a MapStep).
```.profile()``` emits a TraversalMetrics.
```profile('side effect key')``` stores TraversalMetrics in a side effect.
Tests modified and added.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/rjbriody/incubator-tinkerpop TINKERPOP-958
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-tinkerpop/pull/242.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 #242
----
commit 8e807eec73a72b4bbbde3156e6aa8b237d099b42
Author: rjbriody <bo...@datastax.com>
Date: 2016-02-24T20:08:25Z
TINKERPOP-958 split .profile() into ProfileSideEffectStep and ProfileStep (which is a MapStep).
----
---
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-tinkerpop pull request: TINKERPOP-958: split .profile() ...
Posted by dkuppitz <gi...@git.apache.org>.
Github user dkuppitz commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/242#issuecomment-190491649
VOTE: +1
---
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-tinkerpop pull request: TINKERPOP-958: split .profile() ...
Posted by okram <gi...@git.apache.org>.
Github user okram commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/242#issuecomment-191853181
This work has been merged into TINKERPOP-1192 branch. When that branch gets merged to master/, I will close this ticket.
---
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-tinkerpop pull request: TINKERPOP-958: split .profile() ...
Posted by okram <gi...@git.apache.org>.
Github user okram commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/242#issuecomment-188795098
A few things.
1. The name of the test is `ProfileTest.Traversals` not `ProfileSideEffectTest.Traversals`. Its based on the `GraphTraversal.name()`, not the underlying step. Likewise for `GroovyProfileTest.Traversals`. Thus, put all those tests into one test suite. See how there is no `GroupCountSideEffectTest.Traversals`, just `GroupCountTest.Traversals`.
2. Can you provide (as a comment here) the following -- UPGRADE instructions for users and CHANGELOG notes. Don't do it in the code as merging might suck. If you put it here, I can put it in on merge.
3. Can you verify that Giraph/Spark are happy?
Thanks @rjbriody
---
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-tinkerpop pull request: TINKERPOP-958: split .profile() ...
Posted by okram <gi...@git.apache.org>.
Github user okram commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/242#issuecomment-190488986
When this gets merged, I will handle getting it to work with the `Memory` model in https://github.com/apache/incubator-tinkerpop/pull/243 .. will be a good test to see how easy it is for people who have MapReduce-based steps (basically no one) to convert to `MemoryComputeKey`.
---
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-tinkerpop pull request: TINKERPOP-958: split .profile() ...
Posted by okram <gi...@git.apache.org>.
Github user okram commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/242#issuecomment-189004367
@rjbriody --- is any of this backwards compatible?
---
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-tinkerpop pull request: TINKERPOP-958: split .profile() ...
Posted by rjbriody <gi...@git.apache.org>.
Github user rjbriody commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/242#issuecomment-188858152
1 Done
2
Upgrade Instructions:
The profile()-Step has been refactored into 2 steps - the ProfileStep and the ProfileSideEffectStep. Users who previously used the profile()-Step in conjunction with cap(TraversalMetrics.METRICS_KEY) can now simply omit the cap step. Users who retrieved TraverseralMetrics from the side effects after iteration can still do so, but will need to specify a side effect key when using the profile()-Step.
CHANGELOG
Split existing profile()-Step into ProfileStep and ProfileSideEffectStep. profile()-Step now emits TraversalMetrics without the need for cap()-Step.
3 I'm guessing this is outside the coverage of the tests? I'm not hooked up to run Giraph or Spark (and don't have time to go down that rabbit hole). I could definitely use some help here if testing those is required for merge.
---
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-tinkerpop pull request: TINKERPOP-958: split .profile() ...
Posted by okram <gi...@git.apache.org>.
Github user okram commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/242#issuecomment-189010561
```
Failed tests:
GroovyProfileTest$Traversals>ProfileTest.g_V_hasXinXcreatedX_count_isX1XX_valuesXnameX_profileXmetrics_keyX:358->ProfileTest.validate_g_V_hasXinXcreatedX_count_isX1XX_valuesXnameX_profile:322 There should be 3 top-level metrics. expected:<3> but was:<0>
GroovyProfileTest$Traversals>ProfileTest.g_V_hasXinXcreatedX_count_isX1XX_valuesXnameX_profile:346->ProfileTest.validate_g_V_hasXinXcreatedX_count_isX1XX_valuesXnameX_profile:322 There should be 3 top-level metrics. expected:<3> but was:<0>
ProfileTest$Traversals>ProfileTest.g_V_hasXinXcreatedX_count_isX1XX_valuesXnameX_profileXmetrics_keyX:358->ProfileTest.validate_g_V_hasXinXcreatedX_count_isX1XX_valuesXnameX_profile:322 There should be 3 top-level metrics. expected:<3> but was:<0>
ProfileTest$Traversals>ProfileTest.g_V_hasXinXcreatedX_count_isX1XX_valuesXnameX_profile:346->ProfileTest.validate_g_V_hasXinXcreatedX_count_isX1XX_valuesXnameX_profile:322 There should be 3 top-level metrics. expected:<3> but was:<0>
Tests in error:
GroovyProfileTest$Traversals>ProfileTest.g_V_out_out_profile_modern:156->ProfileTest.validate_g_V_out_out_profile_modern:138 » NullPointer
GroovyProfileTest$Traversals>ProfileTest.g_V_out_out_profile_modernXmetrics_keyX:166->ProfileTest.validate_g_V_out_out_profile_modern:138 » NullPointer
ProfileTest$Traversals>ProfileTest.g_V_out_out_profile_modern:156->ProfileTest.validate_g_V_out_out_profile_modern:138 » NullPointer
ProfileTest$Traversals>ProfileTest.g_V_out_out_profile_modernXmetrics_keyX:166->ProfileTest.validate_g_V_out_out_profile_modern:138 » NullPointer
```
Just right click on `SparkGraphComputerProcessIntegrateTest` and hit "run" (IntelliJ). It runs very fast -- less than 30 seconds.
---
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-tinkerpop pull request: TINKERPOP-958: split .profile() ...
Posted by okram <gi...@git.apache.org>.
Github user okram commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/242#issuecomment-189401029
VOTE +1.
---
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-tinkerpop pull request: TINKERPOP-958: split .profile() ...
Posted by rjbriody <gi...@git.apache.org>.
Github user rjbriody commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/242#issuecomment-190730700
Docs have been updated but I can't process them because I don't have Hadoop running so I only did a dry run.
---
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-tinkerpop pull request: TINKERPOP-958: split .profile() ...
Posted by rjbriody <gi...@git.apache.org>.
Github user rjbriody commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/242#issuecomment-189312044
Spark tests pass.
```
mvn clean install verify -DskipIntegrationTests=false
```
...running locally, but could be a while. CI may finish first.
---
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-tinkerpop pull request: TINKERPOP-958: split .profile() ...
Posted by rjbriody <gi...@git.apache.org>.
Github user rjbriody commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/242#issuecomment-189005146
Nope.
These are the 2 things possible before:
```
t = g.V().profile()
t.iterate()
metrics = t.asAdmin().getSideEffects().get(TraversalMetrics.METRICS_KEY).get()
```
or
```
g.V().profile().cap(TraversalMetrics.METRICS_KEY)
```
Neither of which behave the same way now.
---
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-tinkerpop pull request: TINKERPOP-958: split .profile() ...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/242#issuecomment-190713524
i don't see any changes to docs here. I assume at minimum "upgrade docs" are needed especially since this is a breaking change. @okram will you handle that on merge?
VOTE +1 assuming the addition of docs
---
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-tinkerpop pull request: TINKERPOP-958: split .profile() ...
Posted by rjbriody <gi...@git.apache.org>.
Github user rjbriody commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/242#issuecomment-190714200
I left upgrade and changelog notes above: https://github.com/apache/incubator-tinkerpop/pull/242#issuecomment-188858152
...but you just made me remember that the docs for Profile Step need to be updated as well. @okram, should I make those in place as part of this PR or provide notes for what to change?
---
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-tinkerpop pull request: TINKERPOP-958: split .profile() ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-tinkerpop/pull/242
---
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-tinkerpop pull request: TINKERPOP-958: split .profile() ...
Posted by rjbriody <gi...@git.apache.org>.
Github user rjbriody commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/242#issuecomment-189404178
```mvn clean install verify -DskipIntegrationTests=false```
just finished successfully on my machine as well.
---
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-tinkerpop pull request: TINKERPOP-958: split .profile() ...
Posted by okram <gi...@git.apache.org>.
Github user okram commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/242#issuecomment-190737676
I will handle the merge and docs.
---
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.
---