You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apex.apache.org by sandeshh <gi...@git.apache.org> on 2016/07/15 14:43:02 UTC

[GitHub] apex-core pull request #358: *Review only* [APEXCORE-482]Make the downstream...

GitHub user sandeshh opened a pull request:

    https://github.com/apache/apex-core/pull/358

    *Review only* [APEXCORE-482]Make the downstream operator & unifier in the same container as THREA\u2026

    \u2026D_LOCAL
    
    Opening it early, to get the feedback on the approach.
    
    Note:
    Added the locality field. The reason for that is StreamMeta is shared with the upstream operator, so changing the locality in the Unifier affects the upstream as well, but it is not required.
    
    TODO: Add unit tests

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/sandeshh/apex-core APEXCORE-482

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/apex-core/pull/358.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 #358
    
----
commit dbcf00729aaaf595c8d11b020f81a18c5f5a1e42
Author: sandeshh <sa...@gmail.com>
Date:   2016-07-15T14:38:30Z

    Make the downstream operator & unifier in the same container as THREAD_LOCAL

----


---
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] apex-core pull request #358: *Review only* [APEXCORE-482]Make the downstream...

Posted by sandeshh <gi...@git.apache.org>.
Github user sandeshh closed the pull request at:

    https://github.com/apache/apex-core/pull/358


---
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] apex-core pull request #358: *Review only* [APEXCORE-482]Make the downstream...

Posted by gauravgopi123 <gi...@git.apache.org>.
Github user gauravgopi123 commented on a diff in the pull request:

    https://github.com/apache/apex-core/pull/358#discussion_r72494444
  
    --- Diff: engine/src/main/java/com/datatorrent/stram/plan/physical/PTOperator.java ---
    @@ -85,6 +85,7 @@
         private static final long serialVersionUID = 201312112033L;
     
         public final LogicalPlan.StreamMeta logicalStream;
    +    public Locality locality;
    --- End diff --
    
    You need the streammeta between unifier and downstream operator not unifier and upstream operator..


---
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] apex-core issue #358: *Review only* [APEXCORE-482]Make the downstream operat...

Posted by sandeshh <gi...@git.apache.org>.
Github user sandeshh commented on the issue:

    https://github.com/apache/apex-core/pull/358
  
    @tweise I will do the benchmarking. But at the outset, It skips the queue between default unifier and the downstream, that should improve performance.


---
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] apex-core issue #358: *Review only* [APEXCORE-482]Make the downstream operat...

Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on the issue:

    https://github.com/apache/apex-core/pull/358
  
    TODO: provide benchmark numbers for 4, 16, 32 and possibly 64 unifiers comparing existing behavior with behavior after the commit.


---
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] apex-core issue #358: *Review only* [APEXCORE-482]Make the downstream operat...

Posted by tweise <gi...@git.apache.org>.
Github user tweise commented on the issue:

    https://github.com/apache/apex-core/pull/358
  
    That's theoretical the case for the default unifier and possibly not for a different implementation. But in general I would aim for improvements to user relevant scenarios.


---
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] apex-core issue #358: *Review only* [APEXCORE-482]Make the downstream operat...

Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on the issue:

    https://github.com/apache/apex-core/pull/358
  
    Most of the time default unifier (the one that passes tuples unmodified from input streams to an output stream) is used. For that case, it is necessary to benchmark the change (please see my previous TODO comment). In general, there should be a way for an application designer to specify an unifier stream locality for an output port (thread, container, rack or no locality) with some reasonable defaults (whether it is CONTAINER_LOCAL or THREAD_LOCAL). It should be also possible to set not only stream locality for the output port, but also buffer server memory size or queue size. Currently, buffer server queue size when unifier is deployed into a separate container (Nx1 partition) is the same as the original operator buffer server memory. This limitation seems to come from using the same StreamMeta both for the unifier output port and for the partitioned operator output port.


---
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] apex-core issue #358: *Review only* [APEXCORE-482]Make the downstream operat...

Posted by tweise <gi...@git.apache.org>.
Github user tweise commented on the issue:

    https://github.com/apache/apex-core/pull/358
  
    Any benchmarking that demonstrates an improvement or motivates this 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] apex-core pull request #358: *Review only* [APEXCORE-482]Make the downstream...

Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:

    https://github.com/apache/apex-core/pull/358#discussion_r72615867
  
    --- Diff: engine/src/main/java/com/datatorrent/stram/plan/physical/PTOperator.java ---
    @@ -85,6 +85,7 @@
         private static final long serialVersionUID = 201312112033L;
     
         public final LogicalPlan.StreamMeta logicalStream;
    +    public Locality locality;
    --- End diff --
    
    StreamMeta is logical plan object and as there is no unifier in the logical plan, there is only one StreamMeta for the logical stream between upstream and downstream operators.


---
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] apex-core issue #358: *Review only* [APEXCORE-482]Make the downstream operat...

Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on the issue:

    https://github.com/apache/apex-core/pull/358
  
    What is the reason unifier does not have it's own StreamMeta? IMO, it will be better to introduce separate StreamMeta for unifiers instead of the locality field that is used only for unifiers.


---
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] apex-core pull request #358: *Review only* [APEXCORE-482]Make the downstream...

Posted by gauravgopi123 <gi...@git.apache.org>.
Github user gauravgopi123 commented on a diff in the pull request:

    https://github.com/apache/apex-core/pull/358#discussion_r72490248
  
    --- Diff: engine/src/main/java/com/datatorrent/stram/plan/physical/PTOperator.java ---
    @@ -85,6 +85,7 @@
         private static final long serialVersionUID = 201312112033L;
     
         public final LogicalPlan.StreamMeta logicalStream;
    +    public Locality locality;
    --- End diff --
    
    Why can't locality be set on streammeta object?


---
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] apex-core issue #358: *Review only* [APEXCORE-482]Make the downstream operat...

Posted by sandeshh <gi...@git.apache.org>.
Github user sandeshh commented on the issue:

    https://github.com/apache/apex-core/pull/358
  
    @vrozov StreamMeta constructors are private. I will add a "clone" method to that and assign a separate StreamMeta for the unifiers.
    
    Also, I have seen some cases where unifiers are THREAD_LOCAL, will investigate that case as well.
    In the Yahoo Benchmark application, unifiers is CONTAINER_LOCAL.


---
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] apex-core pull request #358: *Review only* [APEXCORE-482]Make the downstream...

Posted by sandeshh <gi...@git.apache.org>.
Github user sandeshh commented on a diff in the pull request:

    https://github.com/apache/apex-core/pull/358#discussion_r72490943
  
    --- Diff: engine/src/main/java/com/datatorrent/stram/plan/physical/PTOperator.java ---
    @@ -85,6 +85,7 @@
         private static final long serialVersionUID = 201312112033L;
     
         public final LogicalPlan.StreamMeta logicalStream;
    +    public Locality locality;
    --- End diff --
    
    StreamMeta is shared with the upstream operator, so changing that has unwanted side effects. I am exploring the options to create a new StreamMeta object for each Unifiers.


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