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 2022/07/13 09:17:59 UTC
[GitHub] [doris] BiteTheDDDDt opened a new pull request, #10809: [Bug][Rollup] fix fe crash because npe on rollupBatchTask
BiteTheDDDDt opened a new pull request, #10809:
URL: https://github.com/apache/doris/pull/10809
# Proposed changes
Issue Number: close #10808
## Problem Summary:
Describe the overview of changes.
## Checklist(Required)
1. Does it affect the original behavior: (Yes/No/I Don't know)
2. Has unit tests been added: (Yes/No/No Need)
3. Has document been added or modified: (Yes/No/No Need)
4. Does it need to update dependencies: (Yes/No)
5. Are there any changes that cannot be rolled back: (Yes/No)
## Further comments
If this is a relatively large or complex change, kick off the discussion at [dev@doris.apache.org](mailto:dev@doris.apache.org) by explaining why you chose the solution you did and what alternatives you considered, etc...
--
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: commits-unsubscribe@doris.apache.org
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] [doris] caiconghui commented on a diff in pull request #10809: [Bug][Rollup] fix fe crash because npe on rollupBatchTask
Posted by GitBox <gi...@apache.org>.
caiconghui commented on code in PR #10809:
URL: https://github.com/apache/doris/pull/10809#discussion_r920697936
##########
fe/fe-core/src/main/java/org/apache/doris/alter/RollupJobV2.java:
##########
@@ -540,7 +540,11 @@ protected boolean cancelImpl(String errMsg) {
private void cancelInternal() {
// clear tasks if has
- AgentTaskQueue.removeBatchTask(rollupBatchTask, TTaskType.ALTER);
+ if (rollupBatchTask != null) {
Review Comment:
rollup batch task should not be null？it should be empty？how to reproduce it？it seems a little serious
rollup batch task now will be always initialized, so it is so strange
--
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: commits-unsubscribe@doris.apache.org
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] [doris] morningman commented on a diff in pull request #10809: [Bug][Rollup] fix fe crash because npe on rollupBatchTask
Posted by GitBox <gi...@apache.org>.
morningman commented on code in PR #10809:
URL: https://github.com/apache/doris/pull/10809#discussion_r920642820
##########
fe/fe-core/src/main/java/org/apache/doris/alter/RollupJobV2.java:
##########
@@ -540,7 +540,11 @@ protected boolean cancelImpl(String errMsg) {
private void cancelInternal() {
// clear tasks if has
- AgentTaskQueue.removeBatchTask(rollupBatchTask, TTaskType.ALTER);
+ if (rollupBatchTask != null) {
Review Comment:
If this is a common case, better not print warn log
--
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: commits-unsubscribe@doris.apache.org
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] [doris] caiconghui commented on a diff in pull request #10809: [Bug][Rollup] fix fe crash because npe on rollupBatchTask
Posted by GitBox <gi...@apache.org>.
caiconghui commented on code in PR #10809:
URL: https://github.com/apache/doris/pull/10809#discussion_r920697936
##########
fe/fe-core/src/main/java/org/apache/doris/alter/RollupJobV2.java:
##########
@@ -540,7 +540,11 @@ protected boolean cancelImpl(String errMsg) {
private void cancelInternal() {
// clear tasks if has
- AgentTaskQueue.removeBatchTask(rollupBatchTask, TTaskType.ALTER);
+ if (rollupBatchTask != null) {
Review Comment:
rollupbatch task should not be null？it should be empty？how to produce it？it seems a little serious
--
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: commits-unsubscribe@doris.apache.org
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] [doris] caiconghui commented on a diff in pull request #10809: [Bug][Rollup] fix fe crash because npe on rollupBatchTask
Posted by GitBox <gi...@apache.org>.
caiconghui commented on code in PR #10809:
URL: https://github.com/apache/doris/pull/10809#discussion_r921258311
##########
fe/fe-core/src/main/java/org/apache/doris/alter/RollupJobV2.java:
##########
@@ -540,7 +540,11 @@ protected boolean cancelImpl(String errMsg) {
private void cancelInternal() {
// clear tasks if has
- AgentTaskQueue.removeBatchTask(rollupBatchTask, TTaskType.ALTER);
+ if (rollupBatchTask != null) {
Review Comment:
> > rollup batch task should not be null？it should be empty？how to reproduce it？it seems a little serious rollup batch task now will be always initialized, so it is so strange
>
> maybe in deserialization， rollup batch task will be null ?
the rollup batch task not the serialization/deserialization filed。 so I think it maybe other proble
##########
fe/fe-core/src/main/java/org/apache/doris/alter/RollupJobV2.java:
##########
@@ -540,7 +540,11 @@ protected boolean cancelImpl(String errMsg) {
private void cancelInternal() {
// clear tasks if has
- AgentTaskQueue.removeBatchTask(rollupBatchTask, TTaskType.ALTER);
+ if (rollupBatchTask != null) {
Review Comment:
> > rollup batch task should not be null？it should be empty？how to reproduce it？it seems a little serious rollup batch task now will be always initialized, so it is so strange
>
> maybe in deserialization， rollup batch task will be null ?
the rollup batch task not the serialization/deserialization filed。 so I think it maybe other problem
--
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: commits-unsubscribe@doris.apache.org
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] [doris] SWJTU-ZhangLei commented on a diff in pull request #10809: [Bug][Rollup] fix fe crash because npe on rollupBatchTask
Posted by GitBox <gi...@apache.org>.
SWJTU-ZhangLei commented on code in PR #10809:
URL: https://github.com/apache/doris/pull/10809#discussion_r920715002
##########
fe/fe-core/src/main/java/org/apache/doris/alter/RollupJobV2.java:
##########
@@ -540,7 +540,11 @@ protected boolean cancelImpl(String errMsg) {
private void cancelInternal() {
// clear tasks if has
- AgentTaskQueue.removeBatchTask(rollupBatchTask, TTaskType.ALTER);
+ if (rollupBatchTask != null) {
Review Comment:
> rollup batch task should not be null？it should be empty？how to reproduce it？it seems a little serious rollup batch task now will be always initialized, so it is so strange
maybe in deserialization， rollup batch task will be null ?
--
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: commits-unsubscribe@doris.apache.org
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] [doris] BiteTheDDDDt commented on a diff in pull request #10809: [Bug][Rollup] fix fe crash because npe on rollupBatchTask
Posted by GitBox <gi...@apache.org>.
BiteTheDDDDt commented on code in PR #10809:
URL: https://github.com/apache/doris/pull/10809#discussion_r920713657
##########
fe/fe-core/src/main/java/org/apache/doris/alter/RollupJobV2.java:
##########
@@ -540,7 +540,11 @@ protected boolean cancelImpl(String errMsg) {
private void cancelInternal() {
// clear tasks if has
- AgentTaskQueue.removeBatchTask(rollupBatchTask, TTaskType.ALTER);
+ if (rollupBatchTask != null) {
Review Comment:
> rollup batch task should not be null？it should be empty？how to reproduce it？it seems a little serious rollup batch task now will be always initialized, so it is so strange
Seem this job object is read from `JournalEntity`.
--
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: commits-unsubscribe@doris.apache.org
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] [doris] BiteTheDDDDt commented on a diff in pull request #10809: [Bug][Rollup] fix fe crash because npe on rollupBatchTask
Posted by GitBox <gi...@apache.org>.
BiteTheDDDDt commented on code in PR #10809:
URL: https://github.com/apache/doris/pull/10809#discussion_r920794079
##########
fe/fe-core/src/main/java/org/apache/doris/alter/RollupJobV2.java:
##########
@@ -540,7 +540,11 @@ protected boolean cancelImpl(String errMsg) {
private void cancelInternal() {
// clear tasks if has
- AgentTaskQueue.removeBatchTask(rollupBatchTask, TTaskType.ALTER);
+ if (rollupBatchTask != null) {
Review Comment:
Maybe it is because we write sc/rollup job as AlterJobV2, this base class not have `rollupBatchTask`/`schemaChangeBatchTask`
--
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: commits-unsubscribe@doris.apache.org
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] [doris] BiteTheDDDDt commented on pull request #10809: [Bug][Rollup] fix fe crash because npe on rollupBatchTask
Posted by GitBox <gi...@apache.org>.
BiteTheDDDDt commented on PR #10809:
URL: https://github.com/apache/doris/pull/10809#issuecomment-1186506164
fixed on https://github.com/apache/doris/pull/10943
--
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: commits-unsubscribe@doris.apache.org
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] [doris] caiconghui commented on a diff in pull request #10809: [Bug][Rollup] fix fe crash because npe on rollupBatchTask
Posted by GitBox <gi...@apache.org>.
caiconghui commented on code in PR #10809:
URL: https://github.com/apache/doris/pull/10809#discussion_r921260814
##########
fe/fe-core/src/main/java/org/apache/doris/alter/RollupJobV2.java:
##########
@@ -540,7 +540,11 @@ protected boolean cancelImpl(String errMsg) {
private void cancelInternal() {
// clear tasks if has
- AgentTaskQueue.removeBatchTask(rollupBatchTask, TTaskType.ALTER);
+ if (rollupBatchTask != null) {
Review Comment:
> Maybe it is because we write sc/rollup job as AlterJobV2, this base class not have `rollupBatchTask`/`schemaChangeBatchTask`
hi @BiteTheDDDDt is there a way to reproduce this bug。 AlterJobV2 is abstract class，once created，it must be RollupJobV2 or SchemaChangeJobV2， so I don't think we find the real cause.
--
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: commits-unsubscribe@doris.apache.org
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] [doris] BiteTheDDDDt closed pull request #10809: [Bug][Rollup] fix fe crash because npe on rollupBatchTask
Posted by GitBox <gi...@apache.org>.
BiteTheDDDDt closed pull request #10809: [Bug][Rollup] fix fe crash because npe on rollupBatchTask
URL: https://github.com/apache/doris/pull/10809
--
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: commits-unsubscribe@doris.apache.org
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] [doris] caiconghui commented on a diff in pull request #10809: [Bug][Rollup] fix fe crash because npe on rollupBatchTask
Posted by GitBox <gi...@apache.org>.
caiconghui commented on code in PR #10809:
URL: https://github.com/apache/doris/pull/10809#discussion_r921260814
##########
fe/fe-core/src/main/java/org/apache/doris/alter/RollupJobV2.java:
##########
@@ -540,7 +540,11 @@ protected boolean cancelImpl(String errMsg) {
private void cancelInternal() {
// clear tasks if has
- AgentTaskQueue.removeBatchTask(rollupBatchTask, TTaskType.ALTER);
+ if (rollupBatchTask != null) {
Review Comment:
> Maybe it is because we write sc/rollup job as AlterJobV2, this base class not have `rollupBatchTask`/`schemaChangeBatchTask`
hi @BiteTheDDDDt is there a way to reproduce this bug。 AlterJobV2 is abstract class，and only subclass can write into journal. and the origin design has been implemented so long time, so I am still confused what cause this bug?
--
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: commits-unsubscribe@doris.apache.org
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] [doris] caiconghui commented on a diff in pull request #10809: [Bug][Rollup] fix fe crash because npe on rollupBatchTask
Posted by GitBox <gi...@apache.org>.
caiconghui commented on code in PR #10809:
URL: https://github.com/apache/doris/pull/10809#discussion_r920697936
##########
fe/fe-core/src/main/java/org/apache/doris/alter/RollupJobV2.java:
##########
@@ -540,7 +540,11 @@ protected boolean cancelImpl(String errMsg) {
private void cancelInternal() {
// clear tasks if has
- AgentTaskQueue.removeBatchTask(rollupBatchTask, TTaskType.ALTER);
+ if (rollupBatchTask != null) {
Review Comment:
rollupbatch task should not be null？it should be empty？how to reproduce it？it seems a little serious
--
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: commits-unsubscribe@doris.apache.org
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