[IOTDB-1257] Make a little bit improvement of config and fix some bugs for setting logic

[sunjincheng121]

Change logs：

1. Fix the setting bug, e.g.: RpcTransportFactory.setMaxLength(ioTDBConfig.getThriftMaxFrameSize()); . i.e.,Don't put max_frame_ Size as softmaxlength.
2. Merge initialBufferCapacity and maxFrameSize to thriftDefaultBufferSize. and thriftDefaultBufferSize configable by user.
3. rename the key from initial_buffer_capacity to thrift_default_buffer_capacity
4. rename the key from max_frame_size to thrift_max_frame_size
5. Unified configuration items：

• RpcUtils.java in the service-rpc module depends on tsfile & thrift moudles
• Config.java in the iotdb-jdbc module depends on service-rpc & others moudles
• IoTDBConfig.java in the iotdb-server module depends on service-rpc & iotdb-jdbc moudles

So, such as initial_buffer_Capacity and Max_frame_size config items should be based on rpcutils RpcUtils.java.

There is an open question about #3 & #4. Do we need to modify the variable name, which will affect the compatibility? What do you think?

[ericpai]

LGTM

[sunjincheng121]

Thanks for the review @ericpai ! And I would like to make a little bit improvement later（Improvement of configuration items in IoTDBConfig & Config & RpcUtils）. then would be great if you have time to review the update:) Thank you!

[sunjincheng121]

I would like to review this PR myself first and looking forward the CI results.
And feel free to left your comments after CI passed. Thank you!

[irvine0109]

Looks good to me.
By the way, I tested this branch on the MacOS of version 10.15.7 and all the test cases passed. So I doubts about the test case of DataClientProviderTest.testSyncConcurrency, it may be some fixes to be done.

[sunjincheng121]

Thanks for your feedback! @irvine0109 ! I had run the test in local and rerun the CI check, then the test case of testSyncConcurrency has passed.

@qiaojialin @jixuan1989 @jt2594838 would be great to have your comments in this PR. :)

[jixuan1989]

The only concern from me is the default_buffer size is set as 16MB while it was 64KB.
Whether it has side-effect depends on:

1. How Thrift runs? will it generate a defined (not cached) buffer (64KB or 16MB) for each request?
2. Will IoTDB generate some small RPC requests/responses frequently?

[jixuan1989]

as service-rpc/src/main/java/org/apache/iotdb/rpc/TElasticFramedTransport.java is modified, I asked @neuyilan to have a review.

[neuyilan]

Thanks for your efforts.
But I have the same concerning problems with @jixuan1989, why we change the default thrift initial buffer size 64KB to 16MB?

There are some requests only need minor thrift buffer sizes, such as heartbeat, match term request, request commit id when syncing leader, and so on. if we change the default size to 16MB, it may be wast some memory. As far as I know, thrift declares a 16MB memory array at initialization (if the initial size is set to 16MB).

In fact, according to our previous experience, only when a snapshot occurs, the request will be larger than 16MB, because a single raft log cannot be larger than 16MB (WAL buffer size is only 8MB by default, if it is larger than 16MB, an error will be reported). If multiple raft logs exceed 16MB, we have split them into batches and sent them.

[sunjincheng121]

Thanks for the feedback @jixuan1989 @neuyilan

Before this PR, every small request will be expanded to 16m (I guess Jialin's thinking at that time was to avoid frequent expansion caused by the default value of 64K, and expand to 16m at one time), the resize logic is here . but the default 16m will also involve frequent capacity reduction, when frequent (5 times +) small requests comings. Therefore, setting the default values of 64K and 16m is a simple trade-off, because the memory consumption will be dynamically adjusted according to the request size and sizeIfNessessary(). So for me, the default values 64K and 16m are OK from the design point of view (user also can change this default value) . From the perspective of experience you share and the memory resources of the devices, I agree with default 64K, if you prefer to 64K as default value.

I appreciate if you can share some thoughts here @qiaojialin :)

[qiaojialin]

Hi, I'm coming...

Before this PR, every small request will be expanded to 16m

That's right. This is due to that in TElasticFramedTransport, The following code is only for the shrink. However, the resizeIfNecessary() method could expand if the request size < 16m.

if (size < softMaxLength) {
readBuffer.resizeIfNecessary(softMaxLength);
}

Setting the initial value to 16m is better than the current design. Another way is to split the resizeIfNecessary() to expandIfNecessary() and shrinkIfNecessary().

Then:

if (size < softMaxLength) {
readBuffer.shrinkIfNecessary(softMaxLength);
}

[neuyilan]

Another way is to split the resizeIfNecessary() to expandIfNecessary() and shrinkIfNecessary().

This is the essence of the problem. When the current thrift size is less than softMaxLength, calling this function will expand the thrift size to softMaxLength. We only need to separate the expand and shrink methods.

[ericpai]

@qiaojialin @neuyilan

1. I think that the intention of the if block in readFrame() is shrinking, not expanding. Another call of resizeIfNecessary() in readBuffer.fill() can also handle the shrinking if we remove this if block.
2. It doesn't matter how large the default initial buffer size is, 64KB or 16MB. It will be shrinked or expanded in necessary immediately after the call of resizeIfNecessary() in readBuffer.fill(). The initial buffer size will also be set via user's configuration.
3. I also believe that the current design, putting shrinking and expanding logics together in resizeIfNecessary(), is better than separating them in two methods. Separating them means that we push the responsibility of checking the resizing condition up to the caller. The code will become more complex. However, we need a unified rule for resizing the buffer.

If there's any misunderstanding，I'm very glad to receive any feedbacks and details about the design :)

[neuyilan]

@ericpai

1. The original meaning of the resizeifnecessary function is that when the size of my request is larger than the size already declared by thrift, I will try to reduce the size; otherwise, I need to expand the size.

2. If the automatic adjustment is correct, the initial value of 64KB or 16MB is OK, but if the initial setting is too large, some memory will be wasted before the adjustment ends. At present, the reason why all requests become 16MB is that the parameters passed in by calling the function of resizeifnecessary are incorrect. The parameter to call the resizeifnecessary function should be the size of the request, not softmaxlength. https://github.com/apache/iotdb/blob/master/service-rpc/src/main/java/org/apache/iotdb/rpc/AutoResizingBuffer.java#L50

3. Callers don't need to care about when they need to expand or when they need to shrink. These things are shielded for callers. When the caller starts a read request or ends a written request, just call the following function is ok. What we need to do is to write a good strategy for expansion and reduction.

`
public void resizeIfNecessary(int requestSize) {
int currentCapacity = this.array.length;
if (currentCapacity < size) {
enlargeIfNecessary(size);
} else {
shrinkIfNecessary(size);
}
}

`

[sunjincheng121]

Thank you all for the feedback and discussion. At present, we have basically reached an agreement. Based on our discussion, The resizeifnecessary maintains the encapsulation shrinkage strategy and is transparent to the caller，and we set the default value to 64KB. We can continue to discuss better contraction strategies, and better improvements can be made by opening separate PR. What do you think @neuyilan :)

[neuyilan]

Hi, @sunjincheng121 I think this idea is OK.

[neuyilan]

LGTM

[sonarcloud]

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

25.4% Coverage
0.0% Duplication

[sunjincheng121]

Thank you all! Merging...