[GitHub] [kafka] manijndl7 commented on a change in pull request #9255: MINOR: Consolidate duplicated logic on reset tools

2020-09-28 Thread GitBox 


manijndl7 commented on a change in pull request #9255:
URL: https://github.com/apache/kafka/pull/9255#discussion_r495786493



##
File path: clients/src/main/java/org/apache/kafka/common/utils/Utils.java
##
@@ -1271,4 +1274,34 @@ private static byte checkRange(final byte i) {
 }
 return map;
 }
+
+/**
+ * Convert an ISO8601 based timestamp to an epoch value
+ * @param timestamp to be converted
+ * @return epoch value of a given timestamp
+ * @throws ParseException for timestamp that doesn't follow ISO8601 format
+ */
+public static long getDateTime(String timestamp) throws ParseException {
+final String[] timestampParts = timestamp.split("T");
+if (timestampParts.length < 2) {
+throw new ParseException("Error parsing timestamp. It does not 
contain a 'T' according to ISO8601 format", timestamp.length());
+}
+
+final String secondPart = timestampParts[1];
+if (secondPart == null || secondPart.isEmpty()) {
+throw new ParseException("Error parsing timestamp. Time part after 
'T' is null or empty", timestamp.length());
+}
+
+if (!(secondPart.contains("+") || secondPart.contains("-") || 
secondPart.contains("Z"))) {

Review comment:
   Hi @mjax request you to please review once.





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:
us...@infra.apache.org

[GitHub] [kafka] manijndl7 commented on a change in pull request #9255: MINOR: Consolidate duplicated logic on reset tools

2020-09-16 Thread GitBox 


manijndl7 commented on a change in pull request #9255:
URL: https://github.com/apache/kafka/pull/9255#discussion_r489531741



##
File path: clients/src/main/java/org/apache/kafka/common/utils/Utils.java
##
@@ -1271,4 +1274,34 @@ private static byte checkRange(final byte i) {
 }
 return map;
 }
+
+/**
+ * Convert an ISO8601 based timestamp to an epoch value
+ * @param timestamp to be converted
+ * @return epoch value of a given timestamp
+ * @throws ParseException for timestamp that doesn't follow ISO8601 format
+ */
+public static long getDateTime(String timestamp) throws ParseException {
+final String[] timestampParts = timestamp.split("T");
+if (timestampParts.length < 2) {
+throw new ParseException("Error parsing timestamp. It does not 
contain a 'T' according to ISO8601 format", timestamp.length());
+}
+
+final String secondPart = timestampParts[1];
+if (secondPart == null || secondPart.isEmpty()) {
+throw new ParseException("Error parsing timestamp. Time part after 
'T' is null or empty", timestamp.length());
+}
+
+if (!(secondPart.contains("+") || secondPart.contains("-") || 
secondPart.contains("Z"))) {

Review comment:
   @mjsax please let me know if ur asking anything specific i will try to 
explore that bt following is my understanding about this fxn : -
   
   -call from (streamResetter/ commandgroup) to reset the offset based on 
epooch value given timestamp.
   - check the format is correct if correct else throw exception. 
   - append "Z" for UTC timezone if not there.
   - parse and get the time.
   
   But i tested by running all the unit cases.
   





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:
us...@infra.apache.org

[GitHub] [kafka] manijndl7 commented on a change in pull request #9255: MINOR: Consolidate duplicated logic on reset tools

2020-09-16 Thread GitBox 


manijndl7 commented on a change in pull request #9255:
URL: https://github.com/apache/kafka/pull/9255#discussion_r489534622



##
File path: clients/src/main/java/org/apache/kafka/common/utils/Utils.java
##
@@ -1271,4 +1274,34 @@ private static byte checkRange(final byte i) {
 }
 return map;
 }
+
+/**
+ * Convert an ISO8601 based timestamp to an epoch value
+ * @param timestamp to be converted
+ * @return epoch value of a given timestamp
+ * @throws ParseException for timestamp that doesn't follow ISO8601 format
+ */
+public static long getDateTime(String timestamp) throws ParseException {
+final String[] timestampParts = timestamp.split("T");
+if (timestampParts.length < 2) {
+throw new ParseException("Error parsing timestamp. It does not 
contain a 'T' according to ISO8601 format", timestamp.length());
+}
+
+final String secondPart = timestampParts[1];
+if (secondPart == null || secondPart.isEmpty()) {
+throw new ParseException("Error parsing timestamp. Time part after 
'T' is null or empty", timestamp.length());
+}
+
+if (!(secondPart.contains("+") || secondPart.contains("-") || 
secondPart.contains("Z"))) {

Review comment:
   @mjsax request you to please review this again.
   
   Thanks !!





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:
us...@infra.apache.org

[GitHub] [kafka] manijndl7 commented on a change in pull request #9255: MINOR: Consolidate duplicated logic on reset tools

2020-09-16 Thread GitBox 


manijndl7 commented on a change in pull request #9255:
URL: https://github.com/apache/kafka/pull/9255#discussion_r489515785



##
File path: clients/src/test/java/org/apache/kafka/common/utils/UtilsTest.java
##
@@ -784,4 +787,39 @@ public void testCloseAllQuietly() {
 assertEquals(msg, exception.get().getMessage());
 assertEquals(1, count.get());
 }
+
+@Test
+public void shouldAcceptValidDateFormats() throws ParseException {
+//check valid formats
+invokeGetDateTimeMethod(new 
SimpleDateFormat("-MM-dd'T'HH:mm:ss.SSS"));
+invokeGetDateTimeMethod(new 
SimpleDateFormat("-MM-dd'T'HH:mm:ss.SSSZ"));
+invokeGetDateTimeMethod(new 
SimpleDateFormat("-MM-dd'T'HH:mm:ss.SSSX"));
+invokeGetDateTimeMethod(new 
SimpleDateFormat("-MM-dd'T'HH:mm:ss.SSSXX"));
+invokeGetDateTimeMethod(new 
SimpleDateFormat("-MM-dd'T'HH:mm:ss.SSSXXX"));
+}
+
+@Test
+public void shouldThrowOnInvalidDateFormat() {
+//check some invalid formats
+try {
+invokeGetDateTimeMethod(new 
SimpleDateFormat("-MM-dd'T'HH:mm:ss"));

Review comment:
   agreed , i have made code changes for that please have a look if its 
correct





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:
us...@infra.apache.org

[GitHub] [kafka] manijndl7 commented on a change in pull request #9255: MINOR: Consolidate duplicated logic on reset tools

2020-09-16 Thread GitBox 


manijndl7 commented on a change in pull request #9255:
URL: https://github.com/apache/kafka/pull/9255#discussion_r489515189



##
File path: core/src/main/scala/kafka/admin/ConsumerGroupCommand.scala
##
@@ -823,7 +809,7 @@ object ConsumerGroupCommand extends Logging {
   case (topicPartition, newOffset) => (topicPartition, new 
OffsetAndMetadata(newOffset))
 }
   } else if (opts.options.has(opts.resetToDatetimeOpt)) {
-val timestamp = 
convertTimestamp(opts.options.valueOf(opts.resetToDatetimeOpt))
+val timestamp = 
Utils.getDateTime(opts.options.valueOf(opts.resetToDatetimeOpt))

Review comment:
   @mjsax @guozhangwang request you to please suggest where we can put this 
common code.





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:
us...@infra.apache.org

[GitHub] [kafka] manijndl7 commented on a change in pull request #9255: MINOR: Consolidate duplicated logic on reset tools

2020-09-16 Thread GitBox 


manijndl7 commented on a change in pull request #9255:
URL: https://github.com/apache/kafka/pull/9255#discussion_r489518181



##
File path: clients/src/main/java/org/apache/kafka/common/utils/Utils.java
##
@@ -1271,4 +1274,34 @@ private static byte checkRange(final byte i) {
 }
 return map;
 }
+
+/**
+ * Convert an ISO8601 based timestamp to an epoch value

Review comment:
   Hi @mjsax thanks for reviewing my PR.
   
   I removed "ISO8601" that from the comment as i checked every formatter 
library follow same ISO8601 standard. please let me know ur thoughts on that.
   





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:
us...@infra.apache.org

[GitHub] [kafka] manijndl7 commented on a change in pull request #9255: MINOR: Consolidate duplicated logic on reset tools

2020-09-16 Thread GitBox 


manijndl7 commented on a change in pull request #9255:
URL: https://github.com/apache/kafka/pull/9255#discussion_r489531741



##
File path: clients/src/main/java/org/apache/kafka/common/utils/Utils.java
##
@@ -1271,4 +1274,34 @@ private static byte checkRange(final byte i) {
 }
 return map;
 }
+
+/**
+ * Convert an ISO8601 based timestamp to an epoch value
+ * @param timestamp to be converted
+ * @return epoch value of a given timestamp
+ * @throws ParseException for timestamp that doesn't follow ISO8601 format
+ */
+public static long getDateTime(String timestamp) throws ParseException {
+final String[] timestampParts = timestamp.split("T");
+if (timestampParts.length < 2) {
+throw new ParseException("Error parsing timestamp. It does not 
contain a 'T' according to ISO8601 format", timestamp.length());
+}
+
+final String secondPart = timestampParts[1];
+if (secondPart == null || secondPart.isEmpty()) {
+throw new ParseException("Error parsing timestamp. Time part after 
'T' is null or empty", timestamp.length());
+}
+
+if (!(secondPart.contains("+") || secondPart.contains("-") || 
secondPart.contains("Z"))) {

Review comment:
   @mjsax please let me know if ur asking anything specific i will try to 
explore that bt following is my understanding about this fxn : -
   
   -call from (streamResetter/ commandgroup) to reset the offset based on 
epooch value given timestamp.
   - check the format is correct if correct else throw exception. 
   - append "Z" for UTC timezone if not there.
   - parse and get the time.
   
   
   





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:
us...@infra.apache.org

[GitHub] [kafka] manijndl7 commented on a change in pull request #9255: MINOR: Consolidate duplicated logic on reset tools

2020-09-16 Thread GitBox 


manijndl7 commented on a change in pull request #9255:
URL: https://github.com/apache/kafka/pull/9255#discussion_r489518181



##
File path: clients/src/main/java/org/apache/kafka/common/utils/Utils.java
##
@@ -1271,4 +1274,34 @@ private static byte checkRange(final byte i) {
 }
 return map;
 }
+
+/**
+ * Convert an ISO8601 based timestamp to an epoch value

Review comment:
   Hi @mjsax thanks for reviewing my PR.
   
   I removed "ISO8601" that from the comment as it is Defined in very 
Dateformatter. please let me know ur thoughts on that.
   





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:
us...@infra.apache.org

[GitHub] [kafka] manijndl7 commented on a change in pull request #9255: MINOR: Consolidate duplicated logic on reset tools

2020-09-16 Thread GitBox 


manijndl7 commented on a change in pull request #9255:
URL: https://github.com/apache/kafka/pull/9255#discussion_r489515785



##
File path: clients/src/test/java/org/apache/kafka/common/utils/UtilsTest.java
##
@@ -784,4 +787,39 @@ public void testCloseAllQuietly() {
 assertEquals(msg, exception.get().getMessage());
 assertEquals(1, count.get());
 }
+
+@Test
+public void shouldAcceptValidDateFormats() throws ParseException {
+//check valid formats
+invokeGetDateTimeMethod(new 
SimpleDateFormat("-MM-dd'T'HH:mm:ss.SSS"));
+invokeGetDateTimeMethod(new 
SimpleDateFormat("-MM-dd'T'HH:mm:ss.SSSZ"));
+invokeGetDateTimeMethod(new 
SimpleDateFormat("-MM-dd'T'HH:mm:ss.SSSX"));
+invokeGetDateTimeMethod(new 
SimpleDateFormat("-MM-dd'T'HH:mm:ss.SSSXX"));
+invokeGetDateTimeMethod(new 
SimpleDateFormat("-MM-dd'T'HH:mm:ss.SSSXXX"));
+}
+
+@Test
+public void shouldThrowOnInvalidDateFormat() {
+//check some invalid formats
+try {
+invokeGetDateTimeMethod(new 
SimpleDateFormat("-MM-dd'T'HH:mm:ss"));

Review comment:
   i have made code changes for that

##
File path: clients/src/test/java/org/apache/kafka/common/utils/UtilsTest.java
##
@@ -784,4 +787,39 @@ public void testCloseAllQuietly() {
 assertEquals(msg, exception.get().getMessage());
 assertEquals(1, count.get());
 }
+
+@Test
+public void shouldAcceptValidDateFormats() throws ParseException {
+//check valid formats
+invokeGetDateTimeMethod(new 
SimpleDateFormat("-MM-dd'T'HH:mm:ss.SSS"));
+invokeGetDateTimeMethod(new 
SimpleDateFormat("-MM-dd'T'HH:mm:ss.SSSZ"));
+invokeGetDateTimeMethod(new 
SimpleDateFormat("-MM-dd'T'HH:mm:ss.SSSX"));
+invokeGetDateTimeMethod(new 
SimpleDateFormat("-MM-dd'T'HH:mm:ss.SSSXX"));
+invokeGetDateTimeMethod(new 
SimpleDateFormat("-MM-dd'T'HH:mm:ss.SSSXXX"));
+}
+
+@Test
+public void shouldThrowOnInvalidDateFormat() {
+//check some invalid formats
+try {
+invokeGetDateTimeMethod(new 
SimpleDateFormat("-MM-dd'T'HH:mm:ss"));
+fail("Call to getDateTime should fail");
+} catch (final Exception e) {
+e.printStackTrace();

Review comment:
   i have made code changes for that





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:
us...@infra.apache.org

[GitHub] [kafka] manijndl7 commented on a change in pull request #9255: MINOR: Consolidate duplicated logic on reset tools

2020-09-16 Thread GitBox 


manijndl7 commented on a change in pull request #9255:
URL: https://github.com/apache/kafka/pull/9255#discussion_r489515189



##
File path: core/src/main/scala/kafka/admin/ConsumerGroupCommand.scala
##
@@ -823,7 +809,7 @@ object ConsumerGroupCommand extends Logging {
   case (topicPartition, newOffset) => (topicPartition, new 
OffsetAndMetadata(newOffset))
 }
   } else if (opts.options.has(opts.resetToDatetimeOpt)) {
-val timestamp = 
convertTimestamp(opts.options.valueOf(opts.resetToDatetimeOpt))
+val timestamp = 
Utils.getDateTime(opts.options.valueOf(opts.resetToDatetimeOpt))

Review comment:
   @mjsax @guozhangwang can you please suggest where we can put this common 
code.





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:
us...@infra.apache.org

[GitHub] [kafka] manijndl7 commented on a change in pull request #9255: MINOR: Consolidate duplicated logic on reset tools

2020-09-16 Thread GitBox 


manijndl7 commented on a change in pull request #9255:
URL: https://github.com/apache/kafka/pull/9255#discussion_r489515189



##
File path: core/src/main/scala/kafka/admin/ConsumerGroupCommand.scala
##
@@ -823,7 +809,7 @@ object ConsumerGroupCommand extends Logging {
   case (topicPartition, newOffset) => (topicPartition, new 
OffsetAndMetadata(newOffset))
 }
   } else if (opts.options.has(opts.resetToDatetimeOpt)) {
-val timestamp = 
convertTimestamp(opts.options.valueOf(opts.resetToDatetimeOpt))
+val timestamp = 
Utils.getDateTime(opts.options.valueOf(opts.resetToDatetimeOpt))

Review comment:
   @mjsax @guozhangwang please suggest where we can put this common code.





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:
us...@infra.apache.org