[Feature] [Connector-V2] [IoTDB] Added source and sink connectors to support IoTDB 2.X#9872
Conversation
…leCompression in tableSession
|
|
||
| | 数据源 | 支持的版本 | 地址 | | ||
| |-------|------------------------------|----------------| | ||
| | IoTDB | `0.13.0 <= version <= 1.3.X` | localhost:6667 | |
| sinkConfig.setEnableRPCCompression( | ||
| pluginConfig.get(IoTDBSinkOptions.ENABLE_RPC_COMPRESSION)); | ||
| if (pluginConfig.getOptional(IoTDBSinkOptions.CONNECTION_TIMEOUT_IN_MS).isPresent()) { | ||
| checkNotNull(sinkConfig.getEnableRPCCompression()); |
|
|
||
| try { | ||
| if (session != null) { | ||
| session.close(); |
There was a problem hiding this comment.
flush method exception, which can result in the inability to shut down resources
|
@zhangshenghang Hi, thank you for your review and suggestions! I’ve addressed the changes. Could you please take another look? |
| } else { | ||
| session.insertRecords( | ||
| batchRecords.getDeviceIds(), | ||
| batchRecords.getTimestamps(), |
There was a problem hiding this comment.
Should there be a break in this for clause?
|
Please paste the result picture of your local running to check the accuracy. For example, using this task config file |
|
@zhangshenghang Hi! All the checks have passed. |
| | thrift_default_buffer_size | int | No | - | The thrift_default_buffer_size of the IoTDB when you select | | ||
| | thrift_max_frame_size | int | No | - | The thrift max frame size | | ||
| | enable_cache_leader | boolean | No | - | Enable_cache_leader of the IoTDB when you select | | ||
| | version | string | No | - | SQL semantic version used by the client, The possible values are: `V_0_12`, `V_0_13` | |
There was a problem hiding this comment.
Does V2 still works fine in V_0_12, V_0_13? Please check
|
|
||
| > IoTDB 数据接收器 | ||
|
|
||
| ## 描述 |
There was a problem hiding this comment.
Please describe the supported engine.
|
@davidzollo @zhangshenghang Hi! Thank you for the feedback! I've implemented all the suggested changes and passed all the checks. Please review when you have a moment. |
|
I am a little confused. Why is iotdb v2 a separate connector? Can't the original connector directly upgrade its dependencies to be compatible with v2? |
| for (int reader : readers) { | ||
| List<IoTDBv2SourceSplit> assignmentForReader = pendingSplit.remove(reader); | ||
| if (assignmentForReader != null && !assignmentForReader.isEmpty()) { | ||
| log.info("Assign splits {} to reader {}", assignmentForReader, reader); | ||
| try { | ||
| context.assignSplit(reader, assignmentForReader); | ||
| } catch (Exception e) { | ||
| log.error( | ||
| "Failed to assign splits {} to reader {}", | ||
| assignmentForReader, | ||
| reader, | ||
| e); | ||
| pendingSplit.put(reader, assignmentForReader); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
synchronized (stateLock) {
...
}
There was a problem hiding this comment.
In this enumerator class, please guard every access to pendingSplit/shouldEnumerate with the same stateLock—wrap addPendingSplit, assignSplit, addSplitsBack, registerReader, and currentUnassignedSplitSize in that lock
| int readerCount = context.currentParallelism(); | ||
| for (IoTDBv2SourceSplit split : splits) { | ||
| int ownerReader = getSplitOwner(split.splitId(), readerCount); | ||
| log.info("Assigning {} to {} reader.", split, ownerReader); | ||
| pendingSplit.computeIfAbsent(ownerReader, r -> new ArrayList<>()).add(split); | ||
| } |
There was a problem hiding this comment.
pendingSplit is a competitive resource and should be locked,
synchronized (stateLock) {
...
}
Hi @Hisoka-X ! We use a separate connector because each of them supports different IoTDB version, which also requires different dependency to handle things differently. |
|
|
||
| private ITableSession buildTableSession(ReadonlyConfig conf) throws IoTDBConnectionException { | ||
| TableSessionBuilder sessionBuilder = new TableSessionBuilder().enableCompression(false); | ||
| String nodeUrlsString = conf.get(NODE_URLS); |
There was a problem hiding this comment.
It should not be String; it should be List
|
|
||
| | 名称 | 类型 | 是否必填 | 默认值 | 描述 | | ||
| |----------------------------|---------|------|------|----------------------------------------------------------------------------------| | ||
| | node_urls | string | 是 | - | IoTDB 集群地址,格式为 `"host1:port"` 或 `"host1:port,host2:port"` | |
There was a problem hiding this comment.
It should not be String; it should be List, and all other related parts should be modified.
While review the code, I found that a lot of the code is duplicated with the existing code. Can it be optimized? @Hisoka-X @LJW21-02 |
+1 |
| <dependency> | ||
| <groupId>com.timecho.iotdb</groupId> | ||
| <artifactId>shade-iotdb-session</artifactId> | ||
| <version>2.0.6.1</version> | ||
| </dependency> |
There was a problem hiding this comment.
Is shade-iotdb-session not backward compatible?
|
@zhangshenghang Thank you again for the review suggestions! I've made all the changes mentioned above, and the code now passes all tests. |
Hisoka-X
left a comment
There was a problem hiding this comment.
Does V1 have many users? Can we just remove it? After all, we can't support it forever.
Please take a look at this problem。@LJW21-02 I have no other questions. |
|
Hi @Hisoka-X ! Since V1 and V2 are not compatible and some users still rely on V1, we'd recommend keeping it for now to avoid potential issues for existing users. |
我们可以考虑在 Apache IoTDB 1.x 版本不再维护一段时间后移除它,但当前来说(至少 1 年内)它应该都会继续维护 |
|
|
||
| @Override | ||
| public int currentUnassignedSplitSize() { | ||
| return pendingSplit.size(); |
There was a problem hiding this comment.
synchronized (stateLock) {
...
}
| if (!pendingSplit.isEmpty()) { | ||
| assignSplit(Collections.singletonList(subtaskId)); | ||
| } |
There was a problem hiding this comment.
synchronized (stateLock) {
...
}
| for (int reader : readers) { | ||
| List<IoTDBv2SourceSplit> assignmentForReader = pendingSplit.remove(reader); | ||
| if (assignmentForReader != null && !assignmentForReader.isEmpty()) { | ||
| log.info("Assign splits {} to reader {}", assignmentForReader, reader); | ||
| try { | ||
| context.assignSplit(reader, assignmentForReader); | ||
| } catch (Exception e) { | ||
| log.error( | ||
| "Failed to assign splits {} to reader {}", | ||
| assignmentForReader, | ||
| reader, | ||
| e); | ||
| pendingSplit.put(reader, assignmentForReader); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
In this enumerator class, please guard every access to pendingSplit/shouldEnumerate with the same stateLock—wrap addPendingSplit, assignSplit, addSplitsBack, registerReader, and currentUnassignedSplitSize in that lock
Purpose of this pull request
This pull request adds both source and sink connectors to support IoTDB version >= 2.X.
Does this PR introduce any user-facing change?
No
How was this patch tested?
e2e tests
Check list
New License Guide