Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.2.0-incubating
    • Component/s: process
    • Labels:
      None

      Description

      The .profile() step is currently tedious to use. One must remember this entire string to activate profiling and cap off the result: "profile().cap(TraversalMetrics.METRICS_KEY)".

      This should be streamlined.

      Additionally, if the TraversalMetrics are cap'd off the end of the traversal then this step and the duration it takes should be excluded from the metrics and timers. (Moved to own ticket: https://issues.apache.org/jira/browse/TINKERPOP-1078)

        Activity

        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user asfgit closed the pull request at:

        https://github.com/apache/incubator-tinkerpop/pull/242

        Show
        githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/incubator-tinkerpop/pull/242
        Hide
        githubbot ASF GitHub Bot added a comment -

        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.

        Show
        githubbot ASF GitHub Bot added a comment - 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.
        Hide
        githubbot ASF GitHub Bot added a comment -

        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.

        Show
        githubbot ASF GitHub Bot added a comment - 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.
        Hide
        githubbot ASF GitHub Bot added a comment -

        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.

        Show
        githubbot ASF GitHub Bot added a comment - 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.
        Hide
        githubbot ASF GitHub Bot added a comment -

        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?

        Show
        githubbot ASF GitHub Bot added a comment - 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?
        Hide
        githubbot ASF GitHub Bot added a comment -

        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

        Show
        githubbot ASF GitHub Bot added a comment - 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
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user dkuppitz commented on the pull request:

        https://github.com/apache/incubator-tinkerpop/pull/242#issuecomment-190491649

        VOTE: +1

        Show
        githubbot ASF GitHub Bot added a comment - Github user dkuppitz commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/242#issuecomment-190491649 VOTE: +1
        Hide
        githubbot ASF GitHub Bot added a comment -

        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`.

        Show
        githubbot ASF GitHub Bot added a comment - 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`.
        Hide
        githubbot ASF GitHub Bot added a comment -

        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.

        Show
        githubbot ASF GitHub Bot added a comment - 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.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user okram commented on the pull request:

        https://github.com/apache/incubator-tinkerpop/pull/242#issuecomment-189401029

        VOTE +1.

        Show
        githubbot ASF GitHub Bot added a comment - Github user okram commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/242#issuecomment-189401029 VOTE +1.
        Hide
        githubbot ASF GitHub Bot added a comment -

        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.

        Show
        githubbot ASF GitHub Bot added a comment - 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.
        Hide
        githubbot ASF GitHub Bot added a comment -

        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.

        Show
        githubbot ASF GitHub Bot added a comment - 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.
        Hide
        githubbot ASF GitHub Bot added a comment -

        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.

        Show
        githubbot ASF GitHub Bot added a comment - 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.
        Hide
        githubbot ASF GitHub Bot added a comment -

        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?

        Show
        githubbot ASF GitHub Bot added a comment - 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?
        Hide
        githubbot ASF GitHub Bot added a comment -

        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.

        Show
        githubbot ASF GitHub Bot added a comment - 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.
        Hide
        githubbot ASF GitHub Bot added a comment -

        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

        Show
        githubbot ASF GitHub Bot added a comment - 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
        Hide
        githubbot ASF GitHub Bot added a comment -

        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 <bob.briody@datastax.com>
        Date: 2016-02-24T20:08:25Z

        TINKERPOP-958 split .profile() into ProfileSideEffectStep and ProfileStep (which is a MapStep).


        Show
        githubbot ASF GitHub Bot added a comment - 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 <bob.briody@datastax.com> Date: 2016-02-24T20:08:25Z TINKERPOP-958 split .profile() into ProfileSideEffectStep and ProfileStep (which is a MapStep).
        Hide
        okram Marko A. Rodriguez added a comment -

        If you want to pair program, I can do that. If you just want to try and get it going – checkout GroupCountStep and GroupCountSideEffectStep. Those will be the simplests examples that give you the framework for creating the two patterns.

        Show
        okram Marko A. Rodriguez added a comment - If you want to pair program, I can do that. If you just want to try and get it going – checkout GroupCountStep and GroupCountSideEffectStep . Those will be the simplests examples that give you the framework for creating the two patterns.
        Hide
        rjbriody Bob Briody added a comment -

        I don't jive w/ that code either. Profile step injection is ugly business.

        Should be pretty easy to hook this up though since none of the guts need to change. I'm on it.

        Show
        rjbriody Bob Briody added a comment - I don't jive w/ that code either. Profile step injection is ugly business. Should be pretty easy to hook this up though since none of the guts need to change. I'm on it.
        Hide
        okram Marko A. Rodriguez added a comment -

        Great. Please do this ticket and I can help you where you need help. I was looking at your code the other day and its a rats nest for me. I don't jive with your style, soo....

        Show
        okram Marko A. Rodriguez added a comment - Great. Please do this ticket and I can help you where you need help. I was looking at your code the other day and its a rats nest for me. I don't jive with your style, soo....
        Hide
        rjbriody Bob Briody added a comment -

        Yes this sounds right. Bonus: people won't have to remember TraversalMetrics.METRICS_KEY, they just name it whatever sideEffect key they want. I dig it.

        Show
        rjbriody Bob Briody added a comment - Yes this sounds right. Bonus: people won't have to remember TraversalMetrics.METRICS_KEY, they just name it whatever sideEffect key they want. I dig it.
        Hide
        okram Marko A. Rodriguez added a comment -

        Bob Briody I just realized the solution to this.

        groupCount()
        groupCount('m')
        

        Whats the difference? The first is GroupCountStep the second is GroupCountSideEffectStep. The first is a reducer that pops out a Map<Object,Long>, the second is a sideEffect that stores the Map<Object,Long> in traversal.getSideEffects().get('m') and simply passes the incoming traverser to output undisturbed.

        This is how profile() should work:

        profile() -> ProfileStep
        profile('m') -> ProfileSideEffectStep
        

        Then its consistent with the pattern used by other sideEffect/map-steps and it does what you want.

        Show
        okram Marko A. Rodriguez added a comment - Bob Briody I just realized the solution to this. groupCount() groupCount('m') Whats the difference? The first is GroupCountStep the second is GroupCountSideEffectStep . The first is a reducer that pops out a Map<Object,Long> , the second is a sideEffect that stores the Map<Object,Long> in traversal.getSideEffects().get('m') and simply passes the incoming traverser to output undisturbed. This is how profile() should work: profile() -> ProfileStep profile('m') -> ProfileSideEffectStep Then its consistent with the pattern used by other sideEffect/map -steps and it does what you want.
        Hide
        okram Marko A. Rodriguez added a comment -

        Sure. Its your body of work. I don't have strong feelings. Please do what you think is best.

        Show
        okram Marko A. Rodriguez added a comment - Sure. Its your body of work. I don't have strong feelings. Please do what you think is best.
        Hide
        rjbriody Bob Briody added a comment -

        "Additionally, if the TraversalMetrics are cap'd off the end of the traversal then this step and the duration it takes should be excluded from the metrics and timers."

        Marko A. Rodriguez, how do you feel about me splitting off this portion of work into a separate ticket, bangin it out right quick, and PR'ing it to master?

        Show
        rjbriody Bob Briody added a comment - "Additionally, if the TraversalMetrics are cap'd off the end of the traversal then this step and the duration it takes should be excluded from the metrics and timers." Marko A. Rodriguez , how do you feel about me splitting off this portion of work into a separate ticket, bangin it out right quick, and PR'ing it to master?
        Hide
        okram Marko A. Rodriguez added a comment -

        This will be a breaking change and thus, will go into 3.2.0.

        Show
        okram Marko A. Rodriguez added a comment - This will be a breaking change and thus, will go into 3.2.0.
        Hide
        rjbriody Bob Briody added a comment -

        Splitting out .profileWithResults() as a separate step is probably the best option.

        Show
        rjbriody Bob Briody added a comment - Splitting out .profileWithResults() as a separate step is probably the best option.
        Hide
        rjbriody Bob Briody added a comment -

        Adding the TraversalMetrics as the first element would defeat the purpose of having 'profileWithResults' since it would alter the results.

        Show
        rjbriody Bob Briody added a comment - Adding the TraversalMetrics as the first element would defeat the purpose of having 'profileWithResults' since it would alter the results.
        Hide
        okram Marko A. Rodriguez added a comment -

        You know what else we could do for profileWithResults().

        gremlin> g.V.out.profileWithResults()
        ==>Traversal Metrics
        ===========================
        BlahStep
        BlooStep
        ==>v[1]
        ==>v[2]
        ==>v[3]
        

        Thus, you get a Traversal<Vertex,Object>. The first answer is the TraversalMetric. The rest is the query results....

        Show
        okram Marko A. Rodriguez added a comment - You know what else we could do for profileWithResults() . gremlin> g.V.out.profileWithResults() ==>Traversal Metrics =========================== BlahStep BlooStep ==>v[1] ==>v[2] ==>v[3] Thus, you get a Traversal<Vertex,Object> . The first answer is the TraversalMetric . The rest is the query results....
        Hide
        okram Marko A. Rodriguez added a comment -

        I think we should make it like this.

        g.V...out().profile()  --> TraversalMetrics
        

        This does NOT return a traversal, but instead, like explain(), execute the traversal and returns the TraversalMetric. Thus, once you do profile(), the traversal is locked. It MUST be the last "step" in the traversal definition.

        and then we have:

        g.V...out().profileWithResults() --> Traversal<Vertex,Vertex>
        

        The latter is currently what we do with profile() and thus, this will be a breaking change. The TraversalMetrics is retrieved via t.sideEffects().get(..).

        Show
        okram Marko A. Rodriguez added a comment - I think we should make it like this. g.V...out().profile() --> TraversalMetrics This does NOT return a traversal, but instead, like explain() , execute the traversal and returns the TraversalMetric . Thus, once you do profile() , the traversal is locked. It MUST be the last "step" in the traversal definition. and then we have: g.V...out().profileWithResults() --> Traversal<Vertex,Vertex> The latter is currently what we do with profile() and thus, this will be a breaking change. The TraversalMetrics is retrieved via t.sideEffects().get(..) .
        Hide
        rjbriody Bob Briody added a comment -

        I believe the common use-case is to want the TraversalMetrics to be the result of the .profile() step. I do still think it is important to enable users to obtain their regular traversal results and later access the TraversalMetrics as can currently be done via the traversal's side effects post-iteration. However, the common use case should be streamlined.

        One possible, but not fantastic option, would be to have the profile step take a parameter to describe it's behavior, and if not specified it would emit TraversalMetrics. This could be a boolean parameter or possibly a more descriptive constant.

        Show
        rjbriody Bob Briody added a comment - I believe the common use-case is to want the TraversalMetrics to be the result of the .profile() step. I do still think it is important to enable users to obtain their regular traversal results and later access the TraversalMetrics as can currently be done via the traversal's side effects post-iteration. However, the common use case should be streamlined. One possible, but not fantastic option, would be to have the profile step take a parameter to describe it's behavior, and if not specified it would emit TraversalMetrics. This could be a boolean parameter or possibly a more descriptive constant.

          People

          • Assignee:
            rjbriody Bob Briody
            Reporter:
            rjbriody Bob Briody
          • Votes:
            1 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development