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

    https://github.com/apache/spark/pull/15285#discussion_r83813908
  
    --- Diff: core/src/test/scala/org/apache/spark/util/UtilsSuite.scala ---
    @@ -274,65 +276,108 @@ class UtilsSuite extends SparkFunSuite with 
ResetSystemProperties with Logging {
         assert(str(10 * hour + 59 * minute + 59 * second + 999) === "11" + sep 
+ "00 h")
       }
     
    -  test("reading offset bytes of a file") {
    +  def getSuffix(isCompressed: Boolean): String = {
    +    if (isCompressed) {
    +      ".gz"
    +    } else {
    +      ""
    +    }
    +  }
    +
    +  def writeLogFile(path: String, content: Array[Byte]): Unit = {
    +    val outputStream = if (path.endsWith(".gz")) {
    +      new GZIPOutputStream(new FileOutputStream(path))
    +    } else {
    +      new FileOutputStream(path)
    +    }
    +    IOUtils.write(content, outputStream)
    +    outputStream.close()
    +    content.size
    +  }
    +
    +  def testOffsetBytes(isCompressed: Boolean): Unit = {
         val tmpDir2 = Utils.createTempDir()
    -    val f1Path = tmpDir2 + "/f1"
    -    val f1 = new FileOutputStream(f1Path)
    -    
f1.write("1\n2\n3\n4\n5\n6\n7\n8\n9\n".getBytes(StandardCharsets.UTF_8))
    -    f1.close()
    +    val suffix = getSuffix(isCompressed)
    +    val f1Path = tmpDir2 + "/f1" + suffix
    +    writeLogFile(f1Path, 
"1\n2\n3\n4\n5\n6\n7\n8\n9\n".getBytes(StandardCharsets.UTF_8))
    +    val f1Length = Utils.getFileLength(new File(f1Path))
    --- End diff --
    
    Nothing in these tests is testing the cache usage. How about making the 
internal getFileLength completely private (not private[util]) and deal with 
only compressed files (rename to getCompressedFileLenght). And so the only 
publicly visible function would be Utils.getFileLenght(), which is used by both 
LogPage and test code. 
    
    This would ensure that we expose only one new public interface in Utils, 
which gets tested thoroughly in the tests.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

Reply via email to