Support replication_num setting for table level #2737
Conversation
| @@ -87,15 +87,13 @@ public short getReplicationNum() { | |||
| return this.properties; | |||
| } | |||
|
|
|||
| public void analyze(int partColNum, Map<String, String> otherProperties) throws AnalysisException { | |||
| public void analyze(int partColNum, Map<String, String> otherProperties, Short replicationNum) throws AnalysisException { | |||
There was a problem hiding this comment.
I don't think it is a good way to analyze this class with a input replicationNum. It is up to the caller to decide how to use this object.
If we do this through input arguments, there will be too many arguments.
There was a problem hiding this comment.
ok, I will fix it
| @@ -148,6 +150,8 @@ public OlapTable() { | |||
| this.indexes = null; | |||
|
|
|||
| this.tableProperty = null; | |||
|
|
|||
| this.replicationNum = null; | |||
There was a problem hiding this comment.
I think you can add this this.replicationNum to the tableProperty so that we do not need to update the FeMetaVersion.
There was a problem hiding this comment.
The current analyzer will remove all the table property after analyzed, include replication_num, so is it a better way to change this design?
There was a problem hiding this comment.
ok. I will fix it
| @@ -3908,6 +3907,12 @@ public static void getDdlStmt(Table table, List<String> createTableStmt, List<St | |||
| sb.append(olapTable.getTableProperty().getDynamicPartitionProperty().toString()); | |||
| } | |||
|
|
|||
| // 7. replicationNum | |||
There was a problem hiding this comment.
Do not add replication num in create table stmt, cause it does not reflect the real replication num of each partition of a table.
There was a problem hiding this comment.
I think just like the DISTRIBUTED BY HASH(id) BUCKETS 5, which also does not reflect the real hash info of each partition of a table. so in the create table it just show the baisc config for the table and user can still set different distribution info for the special partition
the real table status like partition real information should be got by show partitions statement, and create table statement show only show the basic config for the table and the initial status
| @@ -242,6 +242,8 @@ | |||
| import java.util.concurrent.atomic.AtomicBoolean; | |||
| import java.util.concurrent.atomic.AtomicLong; | |||
|
|
|||
| import static org.apache.doris.common.util.PropertyAnalyzer.PROPERTIES_REPLICATION_NUM; | |||
There was a problem hiding this comment.
Do not use static import
| tableProperty.getProperties().put(PropertyAnalyzer.PROPERTIES_REPLICATION_NUM, replicationNum.toString()); | ||
| } | ||
|
|
||
| public Short getReplicationNum() { |
There was a problem hiding this comment.
how about just return FeConstants.default_replication_num is there is no default replica num?
There was a problem hiding this comment.
I think that may confused user. Because the user doesn't set the replication_num in table level,
however, FeConstants.default_replication_num can be changed by admin, and when user don't modify config for table, he may find the replication_num config in table level changed, return null indicate that we don't set any replication_num config in table level, and then we use FeConstants.default_replication_num
| @@ -1203,4 +1203,19 @@ public boolean convertRandomDistributionToHashDistribution() { | |||
| } | |||
| return hasChanged; | |||
| } | |||
|
|
|||
| public void setReplicationNum(Short replicationNum) { | |||
There was a problem hiding this comment.
See comment of TableProperty about how to modify and use this class.
You should add a new field defaultReplicaNum in TableProperty, and this field is built from properties.
Also, a method buildDefaultReplicaNum() should be implemented.
| tableProperty.modifyTableProperties(properties); | ||
| } | ||
| tableProperty.buildReplicationNum(); | ||
| ModifyTablePropertyOperationLog info = new ModifyTablePropertyOperationLog(db.getId(), table.getId(), table.getTableProperty().getProperties()); |
There was a problem hiding this comment.
You do not need to log all properties in TableProperty, only properties is enough
caiconghui
force-pushed
the
replication_num
branch
from
da99b3c to
0a144d8
Compare
Support replication_num setting for table level, so There is no need for user to set replication_num for every alter table add partition statement.