You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@iotdb.apache.org by GitBox <gi...@apache.org> on 2022/04/20 11:31:34 UTC
[GitHub] [iotdb] JackieTien97 opened a new pull request, #5615: Add FragmentInstanceStateMachine for FragmentInstance State change
JackieTien97 opened a new pull request, #5615:
URL: https://github.com/apache/iotdb/pull/5615
1. Let SinkHandle notify FragmentInstanceStateMachine to change to FAILED state while meeting errors in async send event.
2. Change `Driver` from interface to abstract class and extract duplicated code from `DataDriver` and `SchemaDriver` to their parent class `Driver`.
3. Add FragmentInstanceStateMachine to manage FragmentInstance State change
4. Add Inner Exclusive Lock called `DriverLock` in `Driver` to prevent it from closing by others while it is still running.
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] sonarcloud[bot] commented on pull request #5615: Add FragmentInstanceStateMachine for FragmentInstance State change
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #5615:
URL: https://github.com/apache/iotdb/pull/5615#issuecomment-1104651865
SonarCloud Quality Gate failed. [](https://sonarcloud.io/dashboard?id=apache_incubator-iotdb&pullRequest=5615)
[](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=5615&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=5615&resolved=false&types=BUG) [1 Bug](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=5615&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=5615&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=5615&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=5615&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_incubator-iotdb&pullRequest=5615&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_incubator-iotdb&pullRequest=5615&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_incubator-iotdb&pullRequest=5615&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=5615&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=5615&resolved=false&types=CODE_SMELL) [25 Code Smells](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=5615&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_incubator-iotdb&pullRequest=5615&metric=new_coverage&view=list) [46.5% Coverage](https://sonarcloud.io/component_measures?id=apache_incubator-iotdb&pullRequest=5615&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_incubator-iotdb&pullRequest=5615&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_incubator-iotdb&pullRequest=5615&metric=new_duplicated_lines_density&view=list)
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] coveralls commented on pull request #5615: Add FragmentInstanceStateMachine for FragmentInstance State change
Posted by GitBox <gi...@apache.org>.
coveralls commented on PR #5615:
URL: https://github.com/apache/iotdb/pull/5615#issuecomment-1103952434
[](https://coveralls.io/builds/48416547)
Coverage increased (+0.02%) to 54.162% when pulling **06e86e4012ddf927b290359224d8797265f4cf60 on stable-mpp** into **67dafed0e6f1ae7f9bff2d01acd39ca659302719 on master**.
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] JackieTien97 commented on a diff in pull request #5615: Add FragmentInstanceStateMachine for FragmentInstance State change
Posted by GitBox <gi...@apache.org>.
JackieTien97 commented on code in PR #5615:
URL: https://github.com/apache/iotdb/pull/5615#discussion_r854939338
##########
server/src/main/java/org/apache/iotdb/db/mpp/execution/FragmentInstanceContext.java:
##########
@@ -58,10 +64,51 @@ public class FragmentInstanceContext extends QueryContext {
// private final AtomicLong endFullGcCount = new AtomicLong(-1);
// private final AtomicLong endFullGcTimeNanos = new AtomicLong(-1);
- public FragmentInstanceContext(
- FragmentInstanceId id, AtomicReference<FragmentInstanceState> state) {
+ public static FragmentInstanceContext createFragmentInstance(
Review Comment:
fix
##########
server/src/main/java/org/apache/iotdb/db/mpp/execution/FragmentInstanceExecution.java:
##########
@@ -36,23 +37,37 @@ public class FragmentInstanceExecution {
private final Driver driver;
- // TODO we may use StateMachine<FragmentInstanceState> to replace it
- private final AtomicReference<FragmentInstanceState> state;
+ private final ISinkHandle sinkHandle;
+
+ private final FragmentInstanceStateMachine stateMachine;
private long lastHeartbeat;
- public FragmentInstanceExecution(
+ public static FragmentInstanceExecution createFragmentInstanceExecution(
+ IFragmentInstanceScheduler scheduler,
+ FragmentInstanceId instanceId,
+ FragmentInstanceContext context,
+ Driver driver,
+ FragmentInstanceStateMachine stateMachine,
+ CounterStat failedInstances) {
+ FragmentInstanceExecution execution =
+ new FragmentInstanceExecution(scheduler, instanceId, context, driver, stateMachine);
+ execution.initialize(failedInstances, scheduler, instanceId, driver);
Review Comment:
fix
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] sonarcloud[bot] commented on pull request #5615: Add FragmentInstanceStateMachine for FragmentInstance State change
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #5615:
URL: https://github.com/apache/iotdb/pull/5615#issuecomment-1105501875
SonarCloud Quality Gate failed. [](https://sonarcloud.io/dashboard?id=apache_incubator-iotdb&pullRequest=5615)
[](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=5615&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=5615&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=5615&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=5615&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=5615&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=5615&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_incubator-iotdb&pullRequest=5615&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_incubator-iotdb&pullRequest=5615&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_incubator-iotdb&pullRequest=5615&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=5615&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=5615&resolved=false&types=CODE_SMELL) [25 Code Smells](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=5615&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_incubator-iotdb&pullRequest=5615&metric=new_coverage&view=list) [46.3% Coverage](https://sonarcloud.io/component_measures?id=apache_incubator-iotdb&pullRequest=5615&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_incubator-iotdb&pullRequest=5615&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_incubator-iotdb&pullRequest=5615&metric=new_duplicated_lines_density&view=list)
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] xingtanzjr commented on a diff in pull request #5615: Add FragmentInstanceStateMachine for FragmentInstance State change
Posted by GitBox <gi...@apache.org>.
xingtanzjr commented on code in PR #5615:
URL: https://github.com/apache/iotdb/pull/5615#discussion_r854059669
##########
server/src/main/java/org/apache/iotdb/db/mpp/execution/FragmentInstanceContext.java:
##########
@@ -58,10 +64,51 @@ public class FragmentInstanceContext extends QueryContext {
// private final AtomicLong endFullGcCount = new AtomicLong(-1);
// private final AtomicLong endFullGcTimeNanos = new AtomicLong(-1);
- public FragmentInstanceContext(
- FragmentInstanceId id, AtomicReference<FragmentInstanceState> state) {
+ public static FragmentInstanceContext createFragmentInstance(
Review Comment:
`createFragmentInstanceContext`
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] JackieTien97 merged pull request #5615: Add FragmentInstanceStateMachine for FragmentInstance State change
Posted by GitBox <gi...@apache.org>.
JackieTien97 merged PR #5615:
URL: https://github.com/apache/iotdb/pull/5615
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] JackieTien97 commented on a diff in pull request #5615: Add FragmentInstanceStateMachine for FragmentInstance State change
Posted by GitBox <gi...@apache.org>.
JackieTien97 commented on code in PR #5615:
URL: https://github.com/apache/iotdb/pull/5615#discussion_r854687770
##########
server/src/main/java/org/apache/iotdb/db/mpp/execution/FragmentInstanceManager.java:
##########
@@ -148,6 +162,19 @@ public FragmentInstanceInfo abortFragmentInstance(FragmentInstanceId fragmentIns
return null;
}
+ /** Cancels a FragmentInstance. */
+ public FragmentInstanceInfo cancelTask(FragmentInstanceId instanceId) {
Review Comment:
Yes, cancel is called by user kill, abort is called by other FIs' failures in current query.
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] xingtanzjr commented on a diff in pull request #5615: Add FragmentInstanceStateMachine for FragmentInstance State change
Posted by GitBox <gi...@apache.org>.
xingtanzjr commented on code in PR #5615:
URL: https://github.com/apache/iotdb/pull/5615#discussion_r854118794
##########
server/src/main/java/org/apache/iotdb/db/mpp/execution/FragmentInstanceExecution.java:
##########
@@ -36,23 +37,37 @@ public class FragmentInstanceExecution {
private final Driver driver;
- // TODO we may use StateMachine<FragmentInstanceState> to replace it
- private final AtomicReference<FragmentInstanceState> state;
+ private final ISinkHandle sinkHandle;
+
+ private final FragmentInstanceStateMachine stateMachine;
private long lastHeartbeat;
- public FragmentInstanceExecution(
+ public static FragmentInstanceExecution createFragmentInstanceExecution(
+ IFragmentInstanceScheduler scheduler,
+ FragmentInstanceId instanceId,
+ FragmentInstanceContext context,
+ Driver driver,
+ FragmentInstanceStateMachine stateMachine,
+ CounterStat failedInstances) {
+ FragmentInstanceExecution execution =
+ new FragmentInstanceExecution(scheduler, instanceId, context, driver, stateMachine);
+ execution.initialize(failedInstances, scheduler, instanceId, driver);
Review Comment:
It is better understood if `failedInstances` is changed to another name.
`scheduler` `instanceId` `driver` may be duplicated because they have been passed into constructor
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] xingtanzjr commented on a diff in pull request #5615: Add FragmentInstanceStateMachine for FragmentInstance State change
Posted by GitBox <gi...@apache.org>.
xingtanzjr commented on code in PR #5615:
URL: https://github.com/apache/iotdb/pull/5615#discussion_r854126270
##########
server/src/main/java/org/apache/iotdb/db/mpp/execution/FragmentInstanceManager.java:
##########
@@ -148,6 +162,19 @@ public FragmentInstanceInfo abortFragmentInstance(FragmentInstanceId fragmentIns
return null;
}
+ /** Cancels a FragmentInstance. */
+ public FragmentInstanceInfo cancelTask(FragmentInstanceId instanceId) {
Review Comment:
It seems that `abortFragmentInstance()` do the same thing with `cancelTask`
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org