[GitHub] incubator-quarks pull request: [WIP] Added join and joinLast funct...

[dlaboss <gi...@git.apache.org>]

Github user dlaboss commented on a diff in the pull request:

    https://github.com/apache/incubator-quarks/pull/60#discussion_r58201225
  
    --- Diff: spi/topology/src/main/java/quarks/topology/spi/graph/TWindowTimeImpl.java ---
    @@ -94,5 +94,33 @@ Licensed to the Apache Software Foundation (ASF) under one
             
             Aggregate<T,U,K> op = new Aggregate<T,U,K>(window, batcher);
             return feeder().pipe(op); 
    +    }
    +
    +    /**
    +     * @return the time
    +     */
    +    public long getTime() {
    +        return time;
    +    }
    +
    +    /**
    +     * @param time the time to set
    +     */
    --- End diff --
    
    windowing impl newbie wondering about the addition of the setters...  the ctor requires the values be supplied.  Is it really necessary to make them changeable post construction?  They can't be made final?  Changing the values after calling aggregate() or batch() seems to have no affect on the behavior of the windows associated with them.  Maybe just WIP, maybe just need more doc stating that, maybe some class doc clarifying usage of the class - e.g., is a TWindowTimeImpl instance supposed to represent a single window instance or a factory for one or more actual windows (i.e., it's legal to call both aggregate() and batch(), or each one more than once)?


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