sandynz commented on code in PR #21792:
URL: https://github.com/apache/shardingsphere/pull/21792#discussion_r1006759308
##########
kernel/data-pipeline/core/src/main/java/org/apache/shardingsphere/data/pipeline/core/util/PipelineDistributedBarrier.java:
##########
@@ -109,12 +116,13 @@ public void removeParentNode(final String parentPath) {
/**
* Await until all children node is ready.
*
- * @param parentPath parent path
+ * @param jobId job id
* @param timeout timeout
* @param timeUnit time unit
* @return true if the count reached zero and false if the waiting time
elapsed before the count reached zero
*/
- public boolean await(final String parentPath, final long timeout, final
TimeUnit timeUnit) {
+ public boolean await(final String jobId, final long timeout, final
TimeUnit timeUnit) {
+ String parentPath =
PipelineMetaDataNode.getJobBarrierEnablePath(jobId);
Review Comment:
The first parameter is changed from `parentPath` to `jobId`, `parentPath`
might be `enable` or `disable` path, and now it's hard-coded `enable` path, is
it expected?
##########
kernel/data-pipeline/core/src/main/java/org/apache/shardingsphere/data/pipeline/core/job/AbstractPipelineJob.java:
##########
@@ -70,7 +69,7 @@ public Collection<Integer> getShardingItems() {
protected void addTasksRunner(final int shardingItem, final
PipelineTasksRunner tasksRunner) {
tasksRunnerMap.put(shardingItem, tasksRunner);
PipelineJobProgressPersistService.addJobProgressPersistContext(getJobId(),
shardingItem);
-
distributedBarrier.persistEphemeralChildrenNode(PipelineMetaDataNode.getJobBarrierEnablePath(getJobId()),
shardingItem);
+ distributedBarrier.persistEphemeralChildrenNode(getJobId(),
shardingItem);
Review Comment:
Looks `persistEphemeralChildrenNode` first parameter is changed to jobId,
but it might be `enable` or `disable` path before, is it expected? Or is the
impl logic is changed?
##########
kernel/data-pipeline/core/src/main/java/org/apache/shardingsphere/data/pipeline/core/util/PipelineDistributedBarrier.java:
##########
@@ -96,9 +102,10 @@ public void persistEphemeralChildrenNode(final String
parentPath, final int shar
/**
* Persist ephemeral children node.
*
- * @param parentPath parent path
+ * @param jobId job id
*/
- public void removeParentNode(final String parentPath) {
+ public void unregister(final String jobId) {
+ String parentPath =
PipelineMetaDataNode.getJobBarrierDisablePath(jobId);
Review Comment:
`register` method has hard-coded `enable` path, and this `unregister` method
use `disable` path, is it expected?
##########
kernel/data-pipeline/core/src/main/java/org/apache/shardingsphere/data/pipeline/core/prepare/datasource/AbstractDataSourcePreparer.java:
##########
@@ -82,8 +82,8 @@ private void executeCreateSchema(final
PipelineDataSourceManager dataSourceManag
try (Statement statement = connection.createStatement()) {
statement.execute(sql);
}
- } catch (final SQLException ignored) {
- // TODO should not ignore the exception, if do not catch it, the
scaling IT will fail.
+ } catch (final SQLException ex) {
+ log.warn(ex.getMessage());
Review Comment:
Error message could be more meaningful
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
