[CARBONDATA-2440] doc updated to set the property for SDK user #2274

rahulk2 · 2018-05-06T07:51:06Z

Any interfaces changed? NO
Any backward compatibility impacted? No
Document update required? ==> Yes
Testing done ==> All UT and SDV succes report is enough.
For large changes, please consider breaking it into sub-tasks under an umbrella JIRA. NA

ravipesala · 2018-05-06T08:42:04Z

SDV Build Success , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/4745/

CarbonDataQA · 2018-05-06T08:52:41Z

Build Failed with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/5667/

CarbonDataQA · 2018-05-06T08:52:43Z

Build Success with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/4507/

ravipesala · 2018-05-06T09:29:03Z

SDV Build Success , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/4746/

rahulk2 · 2018-05-06T12:44:59Z

retest this please

CarbonDataQA · 2018-05-06T13:38:55Z

Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/5671/

CarbonDataQA · 2018-05-06T13:42:14Z

Build Success with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/4511/

jackylk · 2018-05-09T03:21:32Z

store/sdk/src/main/java/org/apache/carbondata/sdk/file/CarbonWriterBuilder.java

-
+    if (!table.isTransactionalTable()) {
+      CarbonProperties.getInstance()
+          .addProperty(CarbonCommonConstants.ENABLE_OFFHEAP_SORT, "false");


This will impact others, why not let SDK user to set it

Intention was to prevent 'unsafe' property from being used by SDK user by default. Otherwise they have to configure memory for unsafe also. So making it simple to use.

Any way we will update the doc how SDK user can set the property by following code:

CarbonProperties.getInstance() .addProperty("property", "value");

But this will overwrite the ENABLE_OFFHEAP_SORT if the user has set it. Need to check if this is already set. If not then set to false otherwise use what the user has set.

@kunal642 if we are updating the doc to set the properties then no need of this code. same PR we can use to update the doc.

sgururajshetty · 2018-05-11T10:46:10Z

LGTM

CarbonDataQA · 2018-05-11T14:14:09Z

Build Success with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/4686/

CarbonDataQA · 2018-05-11T14:14:09Z

Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/5842/

ravipesala · 2018-05-11T14:33:56Z

SDV Build Success , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/4887/

CarbonDataQA · 2018-05-15T06:56:15Z

Build Failed with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/5883/

ravipesala · 2018-05-15T07:25:27Z

SDV Build Success , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/4918/

CarbonDataQA · 2018-05-15T07:57:17Z

Build Success with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/4731/

ravipesala · 2018-05-15T08:07:33Z

SDV Build Success , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/4919/

CarbonDataQA · 2018-05-15T08:18:48Z

Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/5885/

xubo245 · 2018-05-21T01:14:43Z

docs/sdk-writer-guide.md

+   * This method will be used to add a new property
+   *
+   * @param key
+   * @return properties value


return CarbonProperties object，not properties value

xubo245 · 2018-05-21T01:20:28Z

docs/sdk-writer-guide.md

@@ -32,6 +32,8 @@ These SDK writer output contains just a carbondata and carbonindex files. No met
     fields[1] = new Field("age", DataTypes.INT);

     Schema schema = new Schema(fields);
+
+     CarbonProperties.getInstance().addProperty("enable.offheap.sort", "false");


it should import CarbonProperties first.

this one not fix

xubo245 · 2018-05-21T01:23:03Z

docs/sdk-writer-guide.md

+
+```
+/**
+   * This method will be responsible to get the instance of CarbonProperties class


please optimize the Scala Style: indent

xubo245 · 2018-05-21T01:23:10Z

docs/sdk-writer-guide.md

+
+```
+/**
+   * This method will be used to add a new property


please optimize the Scala Style: indent

xubo245 · 2018-05-21T01:26:12Z

docs/sdk-writer-guide.md

+
+```
+/**
+   * This method will be used to get the property value. if property is not


'if' should be 'If'

xubo245 · 2018-05-21T12:32:57Z

docs/sdk-writer-guide.md

+     if(args[0] != null) {
+     testSdkWriter(args[0]);
+     } else {
+     testSdkWriter("true");


please optimize the Scala Style: indent

xubo245 · 2018-05-21T12:34:01Z

docs/sdk-writer-guide.md

 import org.apache.carbondata.sdk.file.CarbonWriter;
 import org.apache.carbondata.sdk.file.CarbonWriterBuilder;
 import org.apache.carbondata.sdk.file.Field;
 import org.apache.carbondata.sdk.file.Schema;

 public class TestSdk {
-
+
+ // pass true or false whle executing the main to use offheap memory or not


"whle" is incorrect, please optimize.

Suggest use "Pass" instead of "pass".

CarbonDataQA · 2018-05-21T12:40:24Z

Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/6003/

xubo245 · 2018-05-21T12:47:54Z

docs/sdk-writer-guide.md

   public static void main(String[] args) throws IOException, InvalidLoadOptionException {
-     testSdkWriter();
+     if(args[0] != null) {


It will throw "Exception in thread "main" java.lang.ArrayIndexOutOfBoundsException: 0
at org.apache.carbondata.examples.sdk.TestSDK.main(TestSDK.java:17)"

The length of args should more than 0 when you use args[0]

BufferedReader reader = new BufferedReader(new InputStreamReader(System.in)); String enableOffheapSortVal = reader.readLine();

i think above code is ok to take input.

is this ok?

Have you run this test case in you machine? I run it but failed

Didn't you rebase to the latest master? please rebase to the latest code if not

it may be failing because of invalid path given in testSdkWriter().

yeah, this path also has problem. I raised a commit just now:xubo245@09f3877, it can run success, maybe you can refer

xubo245 · 2018-05-21T13:11:52Z

docs/sdk-writer-guide.md

     String path = "/home/root1/Documents/ab/temp";

     Field[] fields = new Field[2];
     fields[0] = new Field("name", DataTypes.STRING);
     fields[1] = new Field("age", DataTypes.INT);

     Schema schema = new Schema(fields);
+
+     CarbonProperties.getInstance().addProperty("enable.offheap.sort", enableOffheap);

     CarbonWriterBuilder builder = CarbonWriter.builder().withSchema(schema).outputPath(path);


some methods have been changed, like withSchema and some. Please rebase to latest code

xubo245 · 2018-05-21T13:21:20Z

docs/sdk-writer-guide.md

   }

-   public static void testSdkWriter() throws IOException, InvalidLoadOptionException {
+   public static void testSdkWriter(String enableOffheap) throws IOException, InvalidLoadOptionException {
     String path = "/home/root1/Documents/ab/temp";


It will throw exception if the path not exists and no permission to create. Can you optimize it? such as change the path.

CarbonDataQA · 2018-05-21T17:34:28Z

Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/6014/

CarbonDataQA · 2018-05-21T18:51:35Z

Build Failed with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/4856/

CarbonDataQA · 2018-05-21T21:22:41Z

Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/6022/

ravipesala · 2018-05-21T21:47:14Z

SDV Build Success , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/5029/

CarbonDataQA · 2018-05-21T22:13:45Z

Build Success with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/4863/

xubo245 · 2018-05-22T01:27:42Z

docs/sdk-writer-guide.md


     Field[] fields = new Field[2];
     fields[0] = new Field("name", DataTypes.STRING);
     fields[1] = new Field("age", DataTypes.INT);

     Schema schema = new Schema(fields);
+
+     CarbonProperties.getInstance().addProperty("enable.offheap.sort", enableOffheap);


The value of "enable.offheap.sort" will transform to false when args[0] not equal to true, including "false" and other string, like "f","any" and so on.

@xubo245 if args[0] is specified as any other value except true or false then it will log warning message and will set default value.

refer

It's different,validateEnableUnsafeSort method validate "enable.unsafe.sort", not "enable.offheap.sort".

xubo245 · 2018-05-22T01:29:28Z

docs/sdk-writer-guide.md

+* This method will be used to add a new property
+*
+* @param key
+* @return CarbonProperties object


should add @param value and describe

xubo245 · 2018-05-22T01:30:30Z

docs/sdk-writer-guide.md

+* present, then it will return the default value.
+*
+* @param key
+* @return properties value


Please add @param defaultValue and related describe.

ravipesala · 2018-05-22T01:57:45Z

SDV Build Success , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/5036/

CarbonDataQA · 2018-05-22T06:17:05Z

Build Failed with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/4874/

CarbonDataQA · 2018-05-22T06:32:40Z

Build Failed with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/6033/

xubo245 · 2018-05-22T07:46:13Z

retest this please

xubo245 · 2018-05-22T09:32:55Z

LGTM

In #2274, the value of enable.offheap.sort will transform to false when args[0] not equal to true, including false and other string, like f,any and so on. So we should validate it. This closes #2331

In apache#2274, the value of enable.offheap.sort will transform to false when args[0] not equal to true, including false and other string, like f,any and so on. So we should validate it. This closes apache#2331

doc updated to set the property for SDK user This closes apache#2274

In apache#2274, the value of enable.offheap.sort will transform to false when args[0] not equal to true, including false and other string, like f,any and so on. So we should validate it. This closes apache#2331

rahulk2 force-pushed the unsafe_mem_sdk branch from 3d67d58 to c8c632e Compare May 6, 2018 07:58

jackylk reviewed May 9, 2018

View reviewed changes

rahulk2 force-pushed the unsafe_mem_sdk branch from c8c632e to 7fe4c87 Compare May 11, 2018 10:41

rahulk2 changed the title ~~[CARBONDATA-2440] default value of ENABLE_OFFHEAP_SORT for sdk set as false~~ [CARBONDATA-2440] doc updated to set the property for SDK user May 11, 2018

rahulk2 force-pushed the unsafe_mem_sdk branch from 7fe4c87 to 8726e9c Compare May 11, 2018 10:45

rahulk2 force-pushed the unsafe_mem_sdk branch 4 times, most recently from 0c31b5b to 9de9ffe Compare May 15, 2018 06:54

xubo245 reviewed May 21, 2018

View reviewed changes

rahulk2 force-pushed the unsafe_mem_sdk branch from 83a7996 to 1e55672 Compare May 21, 2018 12:38

xubo245 reviewed May 21, 2018

View reviewed changes

rahulk2 force-pushed the unsafe_mem_sdk branch from 1e55672 to dd10cfd Compare May 21, 2018 14:24

xubo245 reviewed May 22, 2018

View reviewed changes

[Documentation] document added to set the property for SDK user

2a70c30

rahulk2 force-pushed the unsafe_mem_sdk branch from dd10cfd to 2a70c30 Compare May 22, 2018 05:13

xubo245 mentioned this pull request May 22, 2018

[CARBONDATA-2507] enable.offheap.sort not validate in CarbonData #2331

Closed

5 tasks

asfgit closed this in e1ef85a May 22, 2018

anubhav100 pushed a commit to anubhav100/incubator-carbondata that referenced this pull request Jun 22, 2018

[CARBONDATA-2440] doc updated to set the property for SDK user

1569e51

doc updated to set the property for SDK user This closes apache#2274

[CARBONDATA-2440] doc updated to set the property for SDK user #2274

[CARBONDATA-2440] doc updated to set the property for SDK user #2274

Conversation

rahulk2 commented May 6, 2018 • edited

ravipesala commented May 6, 2018

CarbonDataQA commented May 6, 2018

CarbonDataQA commented May 6, 2018

ravipesala commented May 6, 2018

rahulk2 commented May 6, 2018

CarbonDataQA commented May 6, 2018

CarbonDataQA commented May 6, 2018

Choose a reason for hiding this comment

rahulk2 May 10, 2018 • edited

Choose a reason for hiding this comment

kunal642 May 11, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sgururajshetty commented May 11, 2018

CarbonDataQA commented May 11, 2018

CarbonDataQA commented May 11, 2018

ravipesala commented May 11, 2018

CarbonDataQA commented May 15, 2018

ravipesala commented May 15, 2018

CarbonDataQA commented May 15, 2018

ravipesala commented May 15, 2018

CarbonDataQA commented May 15, 2018

Choose a reason for hiding this comment

xubo245 May 21, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xubo245 May 21, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CarbonDataQA commented May 21, 2018

xubo245 May 21, 2018 • edited

Choose a reason for hiding this comment

rahulk2 May 21, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xubo245 May 21, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xubo245 May 21, 2018 • edited

Choose a reason for hiding this comment

xubo245 May 21, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CarbonDataQA commented May 21, 2018

CarbonDataQA commented May 21, 2018

CarbonDataQA commented May 21, 2018

ravipesala commented May 21, 2018

CarbonDataQA commented May 21, 2018

Choose a reason for hiding this comment

rahulk2 May 22, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ravipesala commented May 22, 2018

CarbonDataQA commented May 22, 2018

CarbonDataQA commented May 22, 2018

xubo245 commented May 22, 2018

xubo245 commented May 22, 2018

rahulk2 commented May 6, 2018 •

edited

rahulk2 May 10, 2018 •

edited

kunal642 May 11, 2018 •

edited

xubo245 May 21, 2018 •

edited

xubo245 May 21, 2018 •

edited

xubo245 May 21, 2018 •

edited

rahulk2 May 21, 2018 •

edited

xubo245 May 21, 2018 •

edited

xubo245 May 21, 2018 •

edited

xubo245 May 21, 2018 •

edited

rahulk2 May 22, 2018 •

edited