-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Integrate the new changes to tasks #13473
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any other tests that should be added with this change?
Should task/backend/bolt be deleted as part of this? Maybe task/backend/inmem too?
@@ -37,7 +37,6 @@ import ( | |||
"github.com/influxdata/influxdb/storage/readservice" | |||
"github.com/influxdata/influxdb/task" | |||
taskbackend "github.com/influxdata/influxdb/task/backend" | |||
taskbolt "github.com/influxdata/influxdb/task/backend/bolt" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is task/backend/bolt still used after this change? If not, can it be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it should be removed now.
@@ -94,7 +94,7 @@ func (as *AnalyticalStorage) FindLogs(ctx context.Context, filter influxdb.LogFi | |||
return nil, 0, err | |||
} | |||
for i := 0; i < len(run.Log); i++ { | |||
logs = append(logs, &run.Log[0]) | |||
logs = append(logs, &run.Log[i]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there be a new or updated test to go with this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. now testing it here 5191978
The failures in litmos and e3e were actually caused by me adjusting on of the tasks_test.go tests to accommodate the new change. I didn't realize at the time that we needed the test to not pass tokens to confirm behavior. As it comes to removing the older pieces, I think we could remove the bolt backend store. I would like to build the cloud pieces first then do a wipe of all the existing stores in one pr. |
* Integrat the new changes to tasks
Closes #