You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@doris.apache.org by GitBox <gi...@apache.org> on 2020/12/28 06:55:53 UTC
[GitHub] [incubator-doris] 924060929 opened a new pull request #5160: [BUG] Follower should forward non-query statement to master repeatedly
924060929 opened a new pull request #5160:
URL: https://github.com/apache/incubator-doris/pull/5160
## Proposed changes
In this case: Follower forward a `insert into` statement to Master, if network interrupted, follower will retry send `insert into` statement in `MasterOpExecutor#forward`, though Master could insert succes at first time, this will cause the data repeat 2 times.
So, wo can retry forward if and only if statement is `QueryStatement`.
## Types of changes
- [x] Bugfix (non-breaking change which fixes an issue)
- [] New feature (non-breaking change which adds functionality)
- [] Breaking change (fix or feature that would cause existing functionality to not work as expected)
- [] Documentation Update (if none of the other choices apply)
- [] Code refactor (Modify the code structure, format the code, etc...)
## Checklist
- [] I have create an issue on (Fix #ISSUE), and have described the bug/feature there in detail
- [x] Compiling and unit tests pass locally with my changes
- [] I have added tests that prove my fix is effective or that my feature works
- [] If this change need a document change, I have updated the document
- [] Any dependent changes have been merged
## Further comments
none
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] 924060929 commented on a change in pull request #5160: [BUG] Follower shouldn't forward non-query statement to master repeatedly
Posted by GitBox <gi...@apache.org>.
924060929 commented on a change in pull request #5160:
URL: https://github.com/apache/incubator-doris/pull/5160#discussion_r549245476
##########
File path: fe/fe-core/src/main/java/org/apache/doris/qe/MasterOpExecutor.java
##########
@@ -42,7 +42,9 @@
// the total time of thrift connectTime add readTime and writeTime
private int thriftTimeoutMs;
- public MasterOpExecutor(OriginStatement originStmt, ConnectContext ctx, RedirectStatus status) {
+ private boolean shouldNotRetry;
+
+ public MasterOpExecutor(OriginStatement originStmt, ConnectContext ctx, RedirectStatus status, boolean isQuery) {
Review comment:
I was discussed this name with my colleague, the constructor of MasterOpExecutor expose 'shouldRetry' could lead to misuse, because could not remember the retry related to statement type when invoke `new MasterOpExecutor()`.
Although `isQuery` doesn't look good, but it always write collect.
And `shouldNotRetry` means exactly that: we should not retry forward to master. `shouldRetry` mismatch the case that `QueryStatement` timeout.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] 924060929 commented on a change in pull request #5160: [BUG] Follower shouldn't forward non-query statement to master repeatedly
Posted by GitBox <gi...@apache.org>.
924060929 commented on a change in pull request #5160:
URL: https://github.com/apache/incubator-doris/pull/5160#discussion_r549254696
##########
File path: fe/fe-core/src/main/java/org/apache/doris/qe/MasterOpExecutor.java
##########
@@ -42,7 +42,9 @@
// the total time of thrift connectTime add readTime and writeTime
private int thriftTimeoutMs;
- public MasterOpExecutor(OriginStatement originStmt, ConnectContext ctx, RedirectStatus status) {
+ private boolean shouldNotRetry;
+
+ public MasterOpExecutor(OriginStatement originStmt, ConnectContext ctx, RedirectStatus status, boolean isQuery) {
Review comment:
the principle is：we must ensure idempotent operation execute at-most-once
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] 924060929 commented on pull request #5160: [BUG] Follower shouldn't forward non-query statement to master repeatedly
Posted by GitBox <gi...@apache.org>.
924060929 commented on pull request #5160:
URL: https://github.com/apache/incubator-doris/pull/5160#issuecomment-751612989
> > > plz add an issue related to this PR
> >
> >
> > done, see #5161
> > what exception will transfer operation throw, when network problem occur and it it not a timeout exception?
TTransportException.UNKNOWN
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] 924060929 commented on a change in pull request #5160: [BUG] Follower shouldn't forward non-query statement to master repeatedly
Posted by GitBox <gi...@apache.org>.
924060929 commented on a change in pull request #5160:
URL: https://github.com/apache/incubator-doris/pull/5160#discussion_r549251871
##########
File path: fe/fe-core/src/main/java/org/apache/doris/qe/MasterOpExecutor.java
##########
@@ -42,7 +42,9 @@
// the total time of thrift connectTime add readTime and writeTime
private int thriftTimeoutMs;
- public MasterOpExecutor(OriginStatement originStmt, ConnectContext ctx, RedirectStatus status) {
+ private boolean shouldNotRetry;
+
+ public MasterOpExecutor(OriginStatement originStmt, ConnectContext ctx, RedirectStatus status, boolean isQuery) {
Review comment:
yes，the condition isQuery is conservative & convenient
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] 924060929 commented on a change in pull request #5160: [BUG] Follower shouldn't forward non-query statement to master repeatedly
Posted by GitBox <gi...@apache.org>.
924060929 commented on a change in pull request #5160:
URL: https://github.com/apache/incubator-doris/pull/5160#discussion_r549245476
##########
File path: fe/fe-core/src/main/java/org/apache/doris/qe/MasterOpExecutor.java
##########
@@ -42,7 +42,9 @@
// the total time of thrift connectTime add readTime and writeTime
private int thriftTimeoutMs;
- public MasterOpExecutor(OriginStatement originStmt, ConnectContext ctx, RedirectStatus status) {
+ private boolean shouldNotRetry;
+
+ public MasterOpExecutor(OriginStatement originStmt, ConnectContext ctx, RedirectStatus status, boolean isQuery) {
Review comment:
I was discussed this name with my colleague, the constructor of MasterOpExecutor expose 'shouldRetry' could lead to misuse, because could not remember the retry related to statement type when invoke `new MasterOpExecutor()`.
Although `isQuery` doesn't look good, but it always write collect.
And `shouldNotRetry` means exactly that: we should not retry forward to 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] caiconghui commented on a change in pull request #5160: [BUG] Follower shouldn't forward non-query statement to master repeatedly
Posted by GitBox <gi...@apache.org>.
caiconghui commented on a change in pull request #5160:
URL: https://github.com/apache/incubator-doris/pull/5160#discussion_r549249878
##########
File path: fe/fe-core/src/main/java/org/apache/doris/qe/MasterOpExecutor.java
##########
@@ -42,7 +42,9 @@
// the total time of thrift connectTime add readTime and writeTime
private int thriftTimeoutMs;
- public MasterOpExecutor(OriginStatement originStmt, ConnectContext ctx, RedirectStatus status) {
+ private boolean shouldNotRetry;
+
+ public MasterOpExecutor(OriginStatement originStmt, ConnectContext ctx, RedirectStatus status, boolean isQuery) {
Review comment:
If we want retry a statement, just need it is a idempotent operation, not just query, right?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] caiconghui edited a comment on pull request #5160: [BUG] Follower shouldn't forward non-query statement to master repeatedly
Posted by GitBox <gi...@apache.org>.
caiconghui edited a comment on pull request #5160:
URL: https://github.com/apache/incubator-doris/pull/5160#issuecomment-751612520
> > plz add an issue related to this PR
>
> done, see #5161
what exception will transfer operation throw, when network problem occur and it it not a timeout exception?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] caiconghui commented on a change in pull request #5160: [BUG] Follower shouldn't forward non-query statement to master repeatedly
Posted by GitBox <gi...@apache.org>.
caiconghui commented on a change in pull request #5160:
URL: https://github.com/apache/incubator-doris/pull/5160#discussion_r549243551
##########
File path: fe/fe-core/src/main/java/org/apache/doris/qe/MasterOpExecutor.java
##########
@@ -42,7 +42,9 @@
// the total time of thrift connectTime add readTime and writeTime
private int thriftTimeoutMs;
- public MasterOpExecutor(OriginStatement originStmt, ConnectContext ctx, RedirectStatus status) {
+ private boolean shouldNotRetry;
+
+ public MasterOpExecutor(OriginStatement originStmt, ConnectContext ctx, RedirectStatus status, boolean isQuery) {
Review comment:
isQuery rename to shouldRetry? and variable 'shouldNotRetry' in MasterOpExecutor rename to "shouldRetry"? I think it is more readable. and then we just need to think when need to retry
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] 924060929 commented on a change in pull request #5160: [BUG] Follower shouldn't forward non-query statement to master repeatedly
Posted by GitBox <gi...@apache.org>.
924060929 commented on a change in pull request #5160:
URL: https://github.com/apache/incubator-doris/pull/5160#discussion_r549254696
##########
File path: fe/fe-core/src/main/java/org/apache/doris/qe/MasterOpExecutor.java
##########
@@ -42,7 +42,9 @@
// the total time of thrift connectTime add readTime and writeTime
private int thriftTimeoutMs;
- public MasterOpExecutor(OriginStatement originStmt, ConnectContext ctx, RedirectStatus status) {
+ private boolean shouldNotRetry;
+
+ public MasterOpExecutor(OriginStatement originStmt, ConnectContext ctx, RedirectStatus status, boolean isQuery) {
Review comment:
the principle is：we must ensure non-idempotent operation execute at-most-once if we has not a same label
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] caiconghui commented on a change in pull request #5160: [BUG] Follower shouldn't forward non-query statement to master repeatedly
Posted by GitBox <gi...@apache.org>.
caiconghui commented on a change in pull request #5160:
URL: https://github.com/apache/incubator-doris/pull/5160#discussion_r549282700
##########
File path: fe/fe-core/src/main/java/org/apache/doris/qe/MasterOpExecutor.java
##########
@@ -42,7 +42,9 @@
// the total time of thrift connectTime add readTime and writeTime
private int thriftTimeoutMs;
- public MasterOpExecutor(OriginStatement originStmt, ConnectContext ctx, RedirectStatus status) {
+ private boolean shouldNotRetry;
+
+ public MasterOpExecutor(OriginStatement originStmt, ConnectContext ctx, RedirectStatus status, boolean isQuery) {
Review comment:
Thank You, I see
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] caiconghui commented on pull request #5160: [BUG] Follower shouldn't forward non-query statement to master repeatedly
Posted by GitBox <gi...@apache.org>.
caiconghui commented on pull request #5160:
URL: https://github.com/apache/incubator-doris/pull/5160#issuecomment-751612520
> > plz add an issue related to this PR
>
> done, see #5161
what exception will transfer operation throw, when network problem occur and it it not a timeout exception?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] 924060929 commented on a change in pull request #5160: [BUG] Follower shouldn't forward non-query statement to master repeatedly
Posted by GitBox <gi...@apache.org>.
924060929 commented on a change in pull request #5160:
URL: https://github.com/apache/incubator-doris/pull/5160#discussion_r549254696
##########
File path: fe/fe-core/src/main/java/org/apache/doris/qe/MasterOpExecutor.java
##########
@@ -42,7 +42,9 @@
// the total time of thrift connectTime add readTime and writeTime
private int thriftTimeoutMs;
- public MasterOpExecutor(OriginStatement originStmt, ConnectContext ctx, RedirectStatus status) {
+ private boolean shouldNotRetry;
+
+ public MasterOpExecutor(OriginStatement originStmt, ConnectContext ctx, RedirectStatus status, boolean isQuery) {
Review comment:
the principle is：we must ensure non-idempotent operation execute at-most-once if we has not a common label
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] caiconghui commented on pull request #5160: [BUG] Follower shouldn't forward non-query statement to master repeatedly
Posted by GitBox <gi...@apache.org>.
caiconghui commented on pull request #5160:
URL: https://github.com/apache/incubator-doris/pull/5160#issuecomment-751605399
plz add an issue related to this PR
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] caiconghui merged pull request #5160: [BUG] Follower shouldn't forward non-query statement to master repeatedly
Posted by GitBox <gi...@apache.org>.
caiconghui merged pull request #5160:
URL: https://github.com/apache/incubator-doris/pull/5160
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] 924060929 commented on pull request #5160: [BUG] Follower shouldn't forward non-query statement to master repeatedly
Posted by GitBox <gi...@apache.org>.
924060929 commented on pull request #5160:
URL: https://github.com/apache/incubator-doris/pull/5160#issuecomment-751609295
> plz add an issue related to this PR
done, see #5161
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] 924060929 commented on a change in pull request #5160: [BUG] Follower shouldn't forward non-query statement to master repeatedly
Posted by GitBox <gi...@apache.org>.
924060929 commented on a change in pull request #5160:
URL: https://github.com/apache/incubator-doris/pull/5160#discussion_r549254696
##########
File path: fe/fe-core/src/main/java/org/apache/doris/qe/MasterOpExecutor.java
##########
@@ -42,7 +42,9 @@
// the total time of thrift connectTime add readTime and writeTime
private int thriftTimeoutMs;
- public MasterOpExecutor(OriginStatement originStmt, ConnectContext ctx, RedirectStatus status) {
+ private boolean shouldNotRetry;
+
+ public MasterOpExecutor(OriginStatement originStmt, ConnectContext ctx, RedirectStatus status, boolean isQuery) {
Review comment:
the principle is：we must ensure non-idempotent operation execute at-most-once
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org