[Bug] Create dynamic partition table failed with enable create_history_partition and not specify the start value by harveyyue · Pull Request #6129 · apache/doris

harveyyue · 2021-06-30T04:54:45Z

fix the issue #5995
Add the property "dynamic_partition.history_partition_num" to specify the history partition number when enable create_history_partition to fix the invalid date format value
and add these two properties to docs

Proposed changes

Describe the big picture of your changes here to communicate to the maintainers why we should accept this pull request. If it fixes a bug or resolves a feature request, be sure to link to that issue.

Types of changes

What types of changes does your code introduce to Doris?
Put an x in the boxes that apply

Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation Update (if none of the other choices apply)
Code refactor (Modify the code structure, format the code, etc...)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

I have created an issue on (Fix [bug] Create dynamic partition table failed with enable create_history_partition and not specify the start value #5995) and described the bug/feature there in detail
Compiling and unit tests pass locally with my changes
I have added tests that prove my fix is effective or that my feature works
If these changes need document changes, I have updated the document
Any dependent changes have been merged

Further comments

If this is a relatively large or complex change, kick off the discussion at dev@doris.apache.org by explaining why you chose the solution you did and what alternatives you considered, etc...

qidaye · 2021-06-30T06:42:17Z

fe/fe-core/src/main/java/org/apache/doris/common/util/DynamicPartitionUtil.java

+
        // Check the number of dynamic partitions that need to be created to avoid creating too many partitions at once.
        // If create_history_partition is false, history partition is not considered.
+        int expect_create_partition_num;


Use camel-case code style

qidaye · 2021-06-30T06:44:49Z

fe/fe-core/src/main/java/org/apache/doris/common/ErrorCode.java

+            "Invalid dynamic partition create_history_partition: %s. Expected true or false"),
+    ERROR_DYNAMIC_PARTITION_HISTORY_PARTITION_NUM_ZERO(5075, new byte[] {'4', '2', '0', '0', '0'},
+            "Dynamic history partition num must greater than 0"),
+    ERROR_DYNAMIC_PARTITION_HISTORY_PARTITION_NUM_FORMAT(5076, new byte[] {'4', '2', '0', '0', '0'},


qidaye · 2021-06-30T06:49:06Z

fe/fe-core/src/main/java/org/apache/doris/clone/DynamicPartitionScheduler.java


        boolean createHistoryPartition = dynamicPartitionProperty.isCreateHistoryPartition();
-        int idx = createHistoryPartition ? dynamicPartitionProperty.getStart() : 0;
+        int idx = createHistoryPartition ? -dynamicPartitionProperty.getHistoryPartitionNum() : 0;


if createHistoryPartition if NOT_SET_HISTORY_PARTITION_NUM idx = start else idx = max(start, -historyPartitionNum) else idx = 0

qidaye · 2021-06-30T06:50:19Z

fe/fe-core/src/main/java/org/apache/doris/catalog/DynamicPartitionProperty.java

            this.buckets = Integer.parseInt(properties.get(BUCKETS));
            this.replicationNum = Integer.parseInt(properties.getOrDefault(REPLICATION_NUM, String.valueOf(NOT_SET_REPLICATION_NUM)));
            this.createHistoryPartition = Boolean.parseBoolean(properties.get(CREATE_HISTORY_PARTITION));
+            this.historyPartitionNum = Integer.parseInt(properties.getOrDefault(HISTORY_PARTITION_NUM, String.valueOf(-start)));


default value should be NOT_SET_HISTORY_PARTITION_NUM, not -start. You can refer to NOT_SET_REPLICATION_NUM.

qidaye · 2021-06-30T07:09:48Z

fe/fe-core/src/main/java/org/apache/doris/common/util/DynamicPartitionUtil.java

            start = 0;
+            expect_create_partition_num = end - start;
+        } else {
+            if (historyPartitionNum != 0) {


Check NOT_SET_HISTORY_PARTITION_NUM

qidaye · 2021-06-30T07:13:25Z

docs/en/sql-reference/sql-statements/Data Definition/CREATE TABLE.md

       Dynamic_partition. Prefix: used to specify the partition name prefix to be created, such as the partition name prefix p, automatically creates the partition name p20200108

       Dynamic_partition. Buckets: specifies the number of partition buckets that are automatically created
+


Consistent with Chinese document format.

qidaye · 2021-06-30T07:28:32Z

fe/fe-core/src/main/java/org/apache/doris/common/util/DynamicPartitionUtil.java

            analyzedProperties.put(DynamicPartitionProperty.CREATE_HISTORY_PARTITION, val);
        }

+        int historyPartitionNum = 0;


It is better to keep the same style as the existing code.
You can refer to REPLICATION_NUM section.

qidaye · 2021-06-30T07:31:47Z

By the way, you also need to update dynamic partition doc https://github.com/apache/incubator-doris/blob/master/docs/en/administrator-guide/dynamic-partition.md, for both Chinese and English.

harveyyue · 2021-06-30T11:45:56Z

By the way, you also need to update dynamic partition doc https://github.com/apache/incubator-doris/blob/master/docs/en/administrator-guide/dynamic-partition.md, for both Chinese and English.

Ok, I have done with these code review issue

qidaye

Thands for your hard work. Left few comments.
Please add some unit tests for your changes.

qidaye · 2021-07-01T12:00:59Z

fe/fe-core/src/main/java/org/apache/doris/clone/DynamicPartitionScheduler.java

 import java.util.Map;
 import java.util.Set;

+import static org.apache.doris.catalog.DynamicPartitionProperty.NOT_SET_HISTORY_PARTITION_NUM;


There is no need to import the static value. You can use it directly. DynamicPartitionProperty.NOT_SET_HISTORY_PARTITION_NUM

qidaye · 2021-07-01T12:04:33Z

fe/fe-core/src/main/java/org/apache/doris/common/util/DynamicPartitionUtil.java

        // Check the number of dynamic partitions that need to be created to avoid creating too many partitions at once.
        // If create_history_partition is false, history partition is not considered.
-        int expect_create_partition_num;
+        int expectCreatePartitionNum;


You could add some logic explaination here.

qidaye · 2021-07-01T12:13:36Z

fe/fe-core/src/main/java/org/apache/doris/common/util/DynamicPartitionUtil.java

-                expect_create_partition_num = end - Math.max(start, -historyPartitionNum);
+            int historyPartitionNum = Integer.valueOf(analyzedProperties.getOrDefault(DynamicPartitionProperty.HISTORY_PARTITION_NUM,
+                    String.valueOf(DynamicPartitionProperty.NOT_SET_HISTORY_PARTITION_NUM)));
+            if (historyPartitionNum > 0) {


Suggested change

if (historyPartitionNum > 0) {

if (historyPartitionNum != DynamicPartitionProperty.NOT_SET_HISTORY_PARTITION_NUM) {

qidaye · 2021-07-01T12:25:15Z

fe/fe-core/src/main/java/org/apache/doris/clone/DynamicPartitionScheduler.java


        boolean createHistoryPartition = dynamicPartitionProperty.isCreateHistoryPartition();
-        int idx = createHistoryPartition ? dynamicPartitionProperty.getStart() : 0;
+        int idx;


Some logical explanations can also be added here.

…y_partition and not specify the start value

qidaye · 2021-07-08T04:34:57Z

LGTM

morningman

LGTM

optimize dynamic partition docs. add feature introduced in #6129

…y_partition and not specify the start value (apache#6129) fix the issue apache#5995 Add the property "dynamic_partition.history_partition_num" to specify the history partition number when enable create_history_partition to fix the invalid date format value and add these two properties to docs

…error (apache#6129) The doris_cloud.outfile for Cloud will show the warning FoundationDB client ignoring unrecognized knob option 'TLS_CERT_REFRESH_DELAY_SECONDS'. This is because FDB configuration is case-sensitive. The option needs to be in all lowercase, so it has been changed to the lowercase configuration.

harveyyue mentioned this pull request Jun 30, 2021

[Bug] Create dynamic partition table failed with enable create_history_partition and not specify the start value #5996

Closed

8 tasks

qidaye reviewed Jun 30, 2021

View reviewed changes

qidaye reviewed Jul 1, 2021

View reviewed changes

harveyyue force-pushed the 5995 branch from dfc9e46 to 4b9cab7 Compare July 5, 2021 09:11

[Bug] Create dynamic partition table failed with enable create_histor…

a89c6ff

…y_partition and not specify the start value

harveyyue force-pushed the 5995 branch from 4b9cab7 to a89c6ff Compare July 5, 2021 09:13

morningman added area/dynamic-partition kind/fix Categorizes issue or PR as related to a bug. labels Jul 9, 2021

morningman approved these changes Jul 9, 2021

View reviewed changes

morningman added the approved Indicates a PR has been approved by one committer. label Jul 9, 2021

morningman merged commit 65892ce into apache:master Jul 10, 2021

qidaye mentioned this pull request Jul 13, 2021

[Doc][DynamicPartition] add dynamic partition creation rules #6217

Merged

13 tasks

morningman pushed a commit that referenced this pull request Jul 14, 2021

[Doc] Optimize dynamic partition docs (#6217)

f4c5c6c

optimize dynamic partition docs. add feature introduced in #6129

		Dynamic_partition. Prefix: used to specify the partition name prefix to be created, such as the partition name prefix p, automatically creates the partition name p20200108

		Dynamic_partition. Buckets: specifies the number of partition buckets that are automatically created

	if (historyPartitionNum > 0) {
	if (historyPartitionNum != DynamicPartitionProperty.NOT_SET_HISTORY_PARTITION_NUM) {

Conversation

harveyyue commented Jun 30, 2021

Proposed changes

Types of changes

Checklist

Further comments

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qidaye commented Jun 30, 2021

Uh oh!

harveyyue commented Jun 30, 2021

Uh oh!

qidaye left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qidaye Jul 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qidaye Jul 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qidaye commented Jul 8, 2021

Uh oh!

morningman left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

qidaye left a comment •

edited

Loading

qidaye Jul 1, 2021 •

edited

Loading

qidaye Jul 1, 2021 •

edited

Loading