[GitHub] [samza] prateekm commented on a change in pull request #1587: SAMZA-2727: Fix UTFDataFormatException for FileUtil

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

prateekm commented on a change in pull request #1587:
URL: https://github.com/apache/samza/pull/1587#discussion_r819804521



##########
File path: samza-core/src/main/scala/org/apache/samza/util/FileUtil.scala
##########
@@ -26,9 +26,13 @@ import java.nio.file._
 import java.util.zip.CRC32
 
 class FileUtil extends Logging {
+  val VERSION = 1

Review comment:
       Where is this used? If not used in the actual serialized data format (might be difficult to do now without breaking backwards compatibility), let's remove from class.

##########
File path: samza-core/src/main/scala/org/apache/samza/util/FileUtil.scala
##########
@@ -26,9 +26,13 @@ import java.nio.file._
 import java.util.zip.CRC32
 
 class FileUtil extends Logging {
+  val VERSION = 1
+  val MAX_WRITE_UTF_SEGMENT = 0xFFFF

Review comment:
       Prefer a human readable val name and value. E.g. `val MaxStringSizeInBytes = 64 * 1024`.
   
   Also, scala convention for statics is PascalCase: https://docs.scala-lang.org/style/naming-conventions.html#constants-values-and-variables

##########
File path: samza-core/src/test/scala/org/apache/samza/util/TestFileUtil.scala
##########
@@ -33,8 +33,9 @@ import scala.util.Random
 class TestFileUtil {
   val data = "100"
   val fileUtil = new FileUtil()
-  val checksum = fileUtil.getChecksum(data)
-  val file = new File(System.getProperty("java.io.tmpdir"), "test")
+  val checksum: Long = fileUtil.getChecksum(data)
+  val TMP_DIR: String = System.getProperty("java.io.tmpdir")

Review comment:
       Minor: Fix case.




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

To unsubscribe, e-mail: commits-unsubscribe@samza.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org