[GitHub] [samza] dnishimura commented on a change in pull request #1016: SAMZA-2179: Move the StartpointVisitor abstraction to SystemAdmin interface.

[GitBox <gi...@apache.org>]

dnishimura commented on a change in pull request #1016: SAMZA-2179: Move the StartpointVisitor abstraction to SystemAdmin interface.
URL: https://github.com/apache/samza/pull/1016#discussion_r281688272
 
 

 ##########
 File path: samza-api/src/main/java/org/apache/samza/startpoint/Startpoint.java
 ##########
 @@ -50,12 +50,13 @@ public long getCreationTimestamp() {
   }
 
   /**
-   * Apply the visitor {@link StartpointVisitor}'s register methods to the instance of this {@link Startpoint}
-   * class.
-   * @param systemStreamPartition The {@link SystemStreamPartition} needed to register with the {@link StartpointVisitor}
-   * @param startpointVisitor The visitor to register with.
+   * Applies the {@link StartpointVisitor}'s visit methods to the {@link Startpoint}
+   * and resolves it to a system specific offset.
+   * @param systemStreamPartition the {@link SystemStreamPartition} of the startpoint.
+   * @param startpointVisitor the visitor of the startpoint.
+   * @return the resolved offset.
    */
-  public abstract void apply(SystemStreamPartition systemStreamPartition, StartpointVisitor startpointVisitor);
+  public abstract String apply(SystemStreamPartition systemStreamPartition, StartpointVisitor startpointVisitor);
 
 Review comment:
   I think for now, generifying the inputs and outputs won't benefit much, but rather will make code readability a little harder. If anything, the output should be explicit as possible or else the interpretation is left to the caller. Also I don't see the input being something other than `SystemStreamPartition` in the future. 
   
   I believe the major motivation for this API change is that other components of Samza that deal with offsets understand string/specific offsets only. IMO, if we want to generify this API, we should take a holistic approach and do it across all offset related components. It'll probably take more careful thought, so could we revisit this approach later?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services