New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[CARBONDATA-3210] Merge common method into CarbonSparkUtil #3032

Closed

xiaohui0318 wants to merge 15 commits into apache:master from xiaohui0318:master

Contributor

xiaohui0318 commented Dec 28, 2018 •

edited

Be sure to do all of the following checklist to help us incorporate
your contribution quickly and easily:

Any interfaces changed?
Any backward compatibility impacted?
Document update required?
Testing done
Please provide details on
- Whether new unit test cases have been added or why no new tests are required?
- How it is tested? Please attach test report.
- Is it a performance related change? Please attach the performance test report.
- Any additional information to help reviewers in testing this change.
For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.

xiaohui0318 added 2 commits

December 28, 2018 14:08


          test commit

b93f75f


          org.apache.carbondata.examples.S3UsingSDKExample#getKeyOnPrefix

c34e88a

org.apache.carbondata.examples.S3Example$#getKeyOnPrefix
org.apache.carbondata.spark.thriftserver.CarbonThriftServer#getKeyOnPrefix
这三个类中的方法getKeyOnPrefix 合并到spark2/src/main/scala/org/apache/carbondata/spark/util/CarbonSparkUtil.scala

CarbonDataQA commented Dec 28, 2018

Can one of the admins verify this patch?

Contributor

qiuchenjian commented Dec 28, 2018

Please describe the change of this PR

qiuchenjian reviewed

View reviewed changes

README.md Outdated

@@ @@ -84,3 +85,6 @@ To get involved in CarbonData: @@
               ## About
               Apache CarbonData is an open source project of The Apache Software Foundation (ASF).
+              ## 2018-12-28开始

Contributor

qiuchenjian Dec 28, 2018

what the usage of this description? why it has chinese?

BeyondYourself reviewed

View reviewed changes

integration/spark2/src/main/scala/org/apache/carbondata/spark/util/CarbonSparkUtil.scala Outdated

+                def getKeyOnPrefix(path: String): (String, String, String) = {
+                  val endPoint = "spark.hadoop." + ENDPOINT
+                  if (path.startsWith(CarbonCommonConstants.S3A_PREFIX)) {
+                    ("spark.hadoop." + ACCESS_KEY, "spark.hadoop." + SECRET_KEY, endPoint)

Contributor

BeyondYourself Dec 28, 2018

Duplicated spark.hadoop." literals make the process of refactoring error-prone, since you must be sure to update all occurrences."，I think you can define a variable uniformly.

xiaohui0318 added 2 commits

December 28, 2018 18:44


          merge common method into CarbonSparkUtil

430ee37


          merge common method into CarbonSparkUtil

846b931

Contributor

xubo245 commented Dec 29, 2018

@xiaohui0318 Please optimize the title, such as change merge to Merge

xubo245 reviewed

View reviewed changes

integration/spark2/src/main/scala/org/apache/carbondata/spark/util/CarbonSparkUtil.scala

@@ @@ -117,4 +116,29 @@ object CarbonSparkUtil { @@
                   case _ =>
                     delimiter
                 }
+                def getKeyOnPrefix(path: String): (String, String, String) = {

Contributor

xubo245 Dec 29, 2018

Please add empty line before this line

xiaohui0318 changed the title ~~[CARBONDATA-3210] merge getKeyOnPrefix into CarbonSparkUtil~~ [CARBONDATA-3210] Merge common method into CarbonSparkUtil

xubo245 reviewed

View reviewed changes

integration/spark2/src/main/scala/org/apache/carbondata/spark/util/CarbonSparkUtil.scala

+                }
+                def getS3EndPoint(args: Array[String]): String = {
+                  if (args.length >= 4 && args(3).contains(".com")) args(3)

Contributor

xubo245 Dec 29, 2018

Can you optimize it? for example: change args to length and endPoint, not args(3). Because endpoint maybe is the 3rd or 4th number of parameter. It will error when the order change in args

xubo245 reviewed

View reviewed changes

integration/spark2/src/main/scala/org/apache/carbondata/spark/util/CarbonSparkUtil.scala Outdated

+                }
+                def getSparkMaster(args: Array[String]): String = {
+                  if (args.length == 6) args(5)

Contributor

xubo245 Dec 29, 2018

Can you optimize it? like the previous comments

xubo245 reviewed

View reviewed changes

integration/spark2/src/main/scala/org/apache/carbondata/spark/util/CarbonSparkUtil.scala Outdated

@@ @@ -62,8 +62,7 @@ object CarbonSparkUtil { @@
                 /**
                  * return's the formatted column comment if column comment is present else empty("")
                  *
-                 * @param carbonColumn
-                 * @return
+                 * @return comment

Contributor

xubo245 Dec 29, 2018

Why do you delete @param carbonColumn?

Contributor

xubo245 commented Dec 29, 2018

Have you validate with S3?

zzcclp reviewed

View reviewed changes

examples/spark2/src/main/scala/org/apache/carbondata/examples/S3UsingSDkExample.scala Outdated

                     .appName("S3UsingSDKExample")
                     .config("spark.driver.host", "localhost")
                     .config(accessKey, args(0))
                     .config(secretKey, args(1))
-                    .config(endpoint, getS3EndPoint(args))
+                    .config(endpoint,CarbonSparkUtil.getS3EndPoint(args))

Contributor

zzcclp Dec 29, 2018

add a blank before CarbonSparkUtil

zzcclp reviewed

View reviewed changes

examples/spark2/src/main/scala/org/apache/carbondata/examples/S3Example.scala Outdated

@@ @@ -21,8 +21,8 @@ import java.io.File @@
               import org.apache.hadoop.fs.s3a.Constants.{ACCESS_KEY, ENDPOINT, SECRET_KEY}
               import org.apache.spark.sql.{Row, SparkSession}
               import org.slf4j.{Logger, LoggerFactory}

Contributor

zzcclp Dec 29, 2018

don't remove this blank line

zzcclp reviewed

View reviewed changes

examples/spark2/src/main/scala/org/apache/carbondata/examples/S3UsingSDkExample.scala Outdated

@@ @@ -20,10 +20,10 @@ import org.apache.hadoop.conf.Configuration @@
               import org.apache.hadoop.fs.s3a.Constants.{ACCESS_KEY, ENDPOINT, SECRET_KEY}
               import org.apache.spark.sql.SparkSession
               import org.slf4j.{Logger, LoggerFactory}

Contributor

zzcclp Dec 29, 2018

don't remove this line

zzcclp reviewed

View reviewed changes

...tion/spark2/src/main/scala/org/apache/carbondata/spark/thriftserver/CarbonThriftServer.scala Outdated

@@ @@ -24,10 +24,10 @@ import org.apache.spark.SparkConf @@
               import org.apache.spark.sql.SparkSession
               import org.apache.spark.sql.hive.thriftserver.HiveThriftServer2
               import org.slf4j.{Logger, LoggerFactory}

Contributor

zzcclp Dec 29, 2018

don't remove

Contributor

xubo245 commented Dec 29, 2018

add to whitelist

CarbonDataQA commented Dec 29, 2018

Build Failed with Spark 2.3.2, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/10336/

CarbonDataQA commented Dec 29, 2018

Build Failed with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/2082/

CarbonDataQA commented Dec 29, 2018

Build Failed with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/2287/


          Merge common metheds and checklist

d642a8c

Contributor Author

xiaohui0318 commented Dec 30, 2018

Be sure to do all of the following checklist to help us incorporate
your contribution quickly and easily:

Any interfaces changed?
NO

Any backward compatibility impacted?
NO

Document update required?
NO

Testing done
NO,only fix style error

For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.
NO

CarbonDataQA commented Dec 30, 2018

Build Failed with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/2088/


          Merge remote-tracking branch 'upstream/master'

71eaddf

CarbonDataQA commented Dec 30, 2018

Build Failed with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/2090/

CarbonDataQA commented Dec 30, 2018

Build Failed with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/2295/

CarbonDataQA commented Dec 30, 2018

Build Failed with Spark 2.3.2, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/10344/


          checklist and fix blank

779c537

CarbonDataQA commented Jan 7, 2019

Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/2182/

CarbonDataQA commented Jan 7, 2019

Build Success with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/2398/

CarbonDataQA commented Jan 7, 2019

Build Success with Spark 2.3.2, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/10438/

zzcclp reviewed

View reviewed changes

examples/spark2/src/main/scala/org/apache/carbondata/examples/S3Example.scala Outdated

    
                  *

                  * @param args require three parameters "Access-key" "Secret-key"

                  *             "table-path on s3" "s3-endpoint" "spark-master"

                  */

Contributor

zzcclp Jan 7, 2019

@xiaohui0318 please check the indent of comments, it needs to remove one blank.
Check other comments.

Contributor Author

xiaohui0318 Jan 8, 2019

checked and fix


          remove comment blank

03ca7b8

CarbonDataQA commented Jan 7, 2019

Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/2185/


          remove comment blank

306030c

CarbonDataQA commented Jan 7, 2019

Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/2187/

CarbonDataQA commented Jan 7, 2019

Build Success with Spark 2.3.2, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/10441/

CarbonDataQA commented Jan 7, 2019

Build Success with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/2403/

CarbonDataQA commented Jan 7, 2019

Build Success with Spark 2.3.2, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/10443/

Contributor

zzcclp commented Jan 8, 2019

LGTM

xubo245 reviewed

View reviewed changes

examples/spark2/src/main/scala/org/apache/carbondata/examples/S3UsingSDkExample.scala

               /**
                * Generate data and write data to S3
                * User can generate different numbers of data by specifying the number-of-rows in parameters
                */
-              object S3UsingSDKExample {
+              object S3UsingSdkExample {

Contributor

xubo245 Jan 8, 2019

Please test it with Huawei OBS

Contributor Author

xiaohui0318 Jan 8, 2019

done

xubo245 reviewed

View reviewed changes

...tion/spark2/src/main/scala/org/apache/carbondata/spark/thriftserver/CarbonThriftServer.scala

    
                * CarbonThriftServer support different modes:

                * 1. read/write data from/to HDFS or local,it only needs configurate storePath

                * 2. read/write data from/to S3, it needs provide access-key, secret-key, s3-endpoint

                */

              object CarbonThriftServer {

Contributor

xubo245 Jan 8, 2019

Please test it with Huawei OBS

Contributor Author

xiaohui0318 Jan 8, 2019

done

xubo245 reviewed

View reviewed changes

examples/spark2/src/main/scala/org/apache/carbondata/examples/S3Example.scala

               import org.apache.spark.sql.{Row, SparkSession}
               import org.slf4j.{Logger, LoggerFactory}
-              import org.apache.carbondata.core.constants.CarbonCommonConstants
+              import org.apache.carbondata.spark.util.CarbonSparkUtil
               object S3Example {

Contributor

xubo245 Jan 8, 2019

Please test it with Huawei OBS

Contributor Author

xiaohui0318 Jan 8, 2019

done


          write data add config and throw Exception

72180d4

CarbonDataQA commented Jan 8, 2019

Build Failed with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/2223/

CarbonDataQA commented Jan 8, 2019

Build Failed with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/2442/

CarbonDataQA commented Jan 8, 2019

Build Failed with Spark 2.3.2, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/10480/


          order the import

8d7eaad

CarbonDataQA commented Jan 8, 2019

Build Failed with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/2225/


          retry order

0fc20c2

CarbonDataQA commented Jan 8, 2019

Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/2226/

CarbonDataQA commented Jan 8, 2019

Build Success with Spark 2.3.2, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/10483/

CarbonDataQA commented Jan 8, 2019

Build Success with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/2445/

Contributor

xubo245 commented Jan 9, 2019

LGTM! Thanks for you contribution!

asfgit closed this in

3a41ee5

asfgit pushed a commit that referenced this pull request


          [CARBONDATA-3210] Merge common method into CarbonSparkUtil and fix ex…

a40c6f1

…ample error

1.merge public methods to spark2/src/main/scala/org/apache/carbondata/spark/util/CarbonSparkUtil.scala
	org.apache.carbondata.examples.S3UsingSDKExample#getKeyOnPrefix
	org.apache.carbondata.examples.S3Example$#getKeyOnPrefix
	org.apache.carbondata.spark.thriftserver.CarbonThriftServer#getKeyOnPrefix

2. fix the error of S3UsingSDKExample

This closes #3032

qiuchenjian pushed a commit to qiuchenjian/carbondata that referenced this pull request


          [CARBONDATA-3210] Merge common method into CarbonSparkUtil and fix ex…

e814d7b

…ample error

1.merge public methods to spark2/src/main/scala/org/apache/carbondata/spark/util/CarbonSparkUtil.scala
	org.apache.carbondata.examples.S3UsingSDKExample#getKeyOnPrefix
	org.apache.carbondata.examples.S3Example$#getKeyOnPrefix
	org.apache.carbondata.spark.thriftserver.CarbonThriftServer#getKeyOnPrefix

2. fix the error of S3UsingSDKExample

This closes apache#3032

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment