[GitHub] carbondata pull request #1841: [CARBONDATA-2062] Configure the temp director...

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/1841
 
    SDV Build Fail , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/3113/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1841
 
    Build Failed  with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/3139/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1841
 
    Build Failed with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/1906/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1841
 
    Build Success with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/1933/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1841
 
    Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/3167/



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/1841
 
    SDV Build Fail , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/3137/



---

[qiuchenjian-2]

Github user sraghunandan commented on the issue:

    https://github.com/apache/carbondata/pull/1841
 
    retest sdv please


---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/1841
 
    SDV Build Success , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/3166/



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1841#discussion_r164347029
 
    --- Diff: integration/spark-common/src/main/scala/org/apache/carbondata/spark/util/CommonUtil.scala ---
    @@ -941,4 +943,40 @@ object CommonUtil {
         }
       }
     
    +  def setTempStoreLocation(
    --- End diff --
   
    Suggesting to add a parameter to use a different temp folder for different operation.
    for data loading, carbon_**dataload**\_\<nanotime\>\_\<split index\>
    for compaction, carbon_**compact**\_\<nanotime\>\_\<split index\>
    for partition, carbon_**partition**\_\<nanotime\>\_\<split index\>
    for streaming handoff, carbon_**handoff**\_\<nanotime\>\_\<split index\>
   



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1841#discussion_r164349728
 
    --- Diff: integration/spark-common/src/main/scala/org/apache/carbondata/spark/util/CommonUtil.scala ---
    @@ -941,4 +943,40 @@ object CommonUtil {
         }
       }
     
    +  def setTempStoreLocation(
    --- End diff --
   
    this is for temporary folder. no need to add the operation to the folder name.Moreover we are using nanotime. which will be different across operations and would not collide


---

[qiuchenjian-2]

Github user QiangCai commented on the issue:

    https://github.com/apache/carbondata/pull/1841
 
    LGTM


---

[qiuchenjian-2]

Github user jackylk commented on the issue:

    https://github.com/apache/carbondata/pull/1841
 
    LGTM


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1841#discussion_r164364870
 
    --- Diff: integration/spark-common/src/main/scala/org/apache/carbondata/spark/util/CommonUtil.scala ---
    @@ -941,4 +943,40 @@ object CommonUtil {
         }
       }
     
    +  def setTempStoreLocation(
    +      index: Int,
    +      carbonLoadModel: CarbonLoadModel,
    +      isCompactionFlow: Boolean,
    +      isAltPartitionFlow: Boolean) : Unit = {
    +    var storeLocation: String = null
    +
    +    // this property is used to determine whether temp location for carbon is inside
    +    // container temp dir or is yarn application directory.
    +    val carbonUseLocalDir = CarbonProperties.getInstance()
    +      .getProperty("carbon.use.local.dir", "false")
    +
    +    if (carbonUseLocalDir.equalsIgnoreCase("true")) {
    +
    +      val storeLocations = Util.getConfiguredLocalDirs(SparkEnv.get.conf)
    +      if (null != storeLocations && storeLocations.nonEmpty) {
    +        storeLocation = storeLocations(Random.nextInt(storeLocations.length))
    +      }
    +      if (storeLocation == null) {
    +        storeLocation = System.getProperty("java.io.tmpdir")
    +      }
    +    } else {
    +      storeLocation = System.getProperty("java.io.tmpdir")
    +    }
    +    storeLocation = storeLocation + CarbonCommonConstants.FILE_SEPARATOR + "carbon" +
    +      System.nanoTime() + CarbonCommonConstants.UNDERSCORE + index
    +
    +    val tempLocationKey = CarbonDataProcessorUtil
    +      .getTempStoreLocationKey(carbonLoadModel.getDatabaseName,
    +        carbonLoadModel.getTableName,
    +        carbonLoadModel.getSegmentId,
    +        carbonLoadModel.getTaskNo,
    +        isCompactionFlow,
    +        isAltPartitionFlow)
    +    CarbonProperties.getInstance().addProperty(tempLocationKey, storeLocation)
    --- End diff --
   
    Do not use CarbonProperty to pass parameter across functions, add this parameter to LoadModel


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1841#discussion_r164371164
 
    --- Diff: integration/spark-common/src/main/scala/org/apache/carbondata/spark/util/CommonUtil.scala ---
    @@ -941,4 +943,40 @@ object CommonUtil {
         }
       }
     
    +  def setTempStoreLocation(
    +      index: Int,
    +      carbonLoadModel: CarbonLoadModel,
    +      isCompactionFlow: Boolean,
    +      isAltPartitionFlow: Boolean) : Unit = {
    +    var storeLocation: String = null
    +
    +    // this property is used to determine whether temp location for carbon is inside
    +    // container temp dir or is yarn application directory.
    +    val carbonUseLocalDir = CarbonProperties.getInstance()
    +      .getProperty("carbon.use.local.dir", "false")
    +
    +    if (carbonUseLocalDir.equalsIgnoreCase("true")) {
    +
    +      val storeLocations = Util.getConfiguredLocalDirs(SparkEnv.get.conf)
    +      if (null != storeLocations && storeLocations.nonEmpty) {
    +        storeLocation = storeLocations(Random.nextInt(storeLocations.length))
    +      }
    +      if (storeLocation == null) {
    +        storeLocation = System.getProperty("java.io.tmpdir")
    +      }
    +    } else {
    +      storeLocation = System.getProperty("java.io.tmpdir")
    +    }
    +    storeLocation = storeLocation + CarbonCommonConstants.FILE_SEPARATOR + "carbon" +
    +      System.nanoTime() + CarbonCommonConstants.UNDERSCORE + index
    +
    +    val tempLocationKey = CarbonDataProcessorUtil
    +      .getTempStoreLocationKey(carbonLoadModel.getDatabaseName,
    +        carbonLoadModel.getTableName,
    +        carbonLoadModel.getSegmentId,
    +        carbonLoadModel.getTaskNo,
    +        isCompactionFlow,
    +        isAltPartitionFlow)
    +    CarbonProperties.getInstance().addProperty(tempLocationKey, storeLocation)
    --- End diff --
   
    CarbonProperty is the methodology used during data loading, compaction. changing alone for streaming handoff will cause confusion and difficult to maintain code due to difference in implementation. your suggested design need to be modified for all flows and needs extensive testing under concurrent scenarios also.In my opinion, that should not be the scope of this PR


---

[qiuchenjian-2]

Github user sraghunandan commented on the issue:

    https://github.com/apache/carbondata/pull/1841
 
    retest this please


---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/1841
 
    SDV Build Success , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/3293/



---

[qiuchenjian-2]

Github user sraghunandan commented on the issue:

    https://github.com/apache/carbondata/pull/1841
 
    retest this please


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1841
 
    Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/3428/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1841
 
    Build Failed with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/2189/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1841
 
    Build Success with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/2191/



---