[GitHub] [samza] cameronlee314 commented on a change in pull request #1117: [SAMZA-2283] Samza Sql Diagnostics: make latency metrics changes backward compatible

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

cameronlee314 commented on a change in pull request #1117: [SAMZA-2283] Samza Sql Diagnostics: make latency metrics changes backward compatible
URL: https://github.com/apache/samza/pull/1117#discussion_r307944744
 
 

 ##########
 File path: samza-api/src/main/java/org/apache/samza/system/IncomingMessageEnvelope.java
 ##########
 @@ -81,8 +81,8 @@ public IncomingMessageEnvelope(SystemStreamPartition systemStreamPartition, Stri
    * @param key A deserialized key received from the partition offset.
    * @param message A deserialized message received from the partition offset.
    * @param size size of the message and key in bytes.
-   * @param eventTime the timestamp (in epochMillis) of when this event happened
-   * @param arrivalTime the timestamp (in epochMillis) of when this event arrived to (i.e., was picked-up by) Samza
+   * @param eventTime the timestamp (in nanoTime) of when this event happened
+   * @param arrivalTime the timestamp (in nanoTime) of when this event arrived to (i.e., was picked-up by) Samza
    */
   public IncomingMessageEnvelope(SystemStreamPartition systemStreamPartition, String offset,
 
 Review comment:
   The constructor is still part of the public API, so it probably is best to add a new constructor as well for specifying nanos. If existing code is building an `IncomingMessageEnvelope` using this constructor, and then calling `getEventTime` and `getArrivalTime`, then they will be getting a value which is too low, since you will divide these values by 1000000.
   Unfortunately, the same constructor signature is desired for both millis and nanos, so maybe you'll need a static method to build the envelope instead of using an actual constructor.
   There may be places outside of Samza which are building `IncomingMessageEnvelope`s (e.g. Brooklin), so if you just change this constructor, then you will need to be very sure that you catch all of the existing usages of this constructor and update them. If you add a new syntax for building `IncomingMessageEnvelope`s for nano times, then existing usages will continue to work as before, and then you can update them to just pick up the more granular functionality.

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