Skip to content

Conversation

@weizuo93
Copy link
Contributor

Proposed changes

Based on PR #4475, this patch add a new feature for single tablet migration between different disks by http.

Types of changes

What types of changes does your code introduce to Doris?
Put an x in the boxes that apply

  • [] 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

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

  • [] I have create an issue on (Fix #ISSUE), and have described the bug/feature there in detail
  • 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

curl -X GET http://be_host:webserver_port/tablet_migration?tablet_id=xxx&schema_hash=xxx&disk=xxx
```

返回值就是tablet迁移结果,迁移成功会返回:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

迁移可能是一个非常长的过程,如果是同步接口的话,可能会导致客户端hang住很长时间?
是否可以考虑做成一个异步接口,调用方只是触发一个 storage_migration的 task放入队列即返回?
然后可以通过接口来查询这个任务是否在运行?类似 http/action/compaction_action 那样?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@morningman morningman added kind/feature Categorizes issue or PR as related to a new feature. api-review Categorizes an issue or PR as actively needing an API review. labels Jan 3, 2021
Submit the migration task:

```
curl -X GET http://be_host:webserver_port/tablet_migration?goal=run&tablet_id=xxx&schema_hash=xxx&disk=xxx
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recommended: /api/tablet_migration?.....

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK.

Show the status of migration task:

```
curl -X GET http://be_host:webserver_port/tablet_migration?goal=status&tablet_id=xxx&schema_hash=xxx&disk=xxx
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recommended: /api/tablet_migration?.....

And only tablet_id and schema_hash is needed.
the result will return which disk it is moved to.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@morningman morningman self-assigned this Jan 12, 2021
}
}
{
std::unique_lock<std::mutex> lock(_migration_status_mutex);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you can move this block to the above code block, to avoid lock _migration_status_mutex twice?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And the "submitted" status seems not necessary? It will be changed to "running" very soon. And it it submit failed, it will be removed from _migration_tasks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you can move this block to the above code block, to avoid lock _migration_status_mutex twice?

OK.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And the "submitted" status seems not necessary? It will be changed to "running" very soon. And it it submit failed, it will be removed from _migration_tasks.

When doing tablet rebalance between disks, there may be many migration tasks being submitted and waiting in the queue. So "submitted" status may be useful.

do {
std::unique_lock<std::mutex> lock(_migration_status_mutex);
std::map<MigrationTask, std::string>::iterator it_task = _migration_tasks.begin();
for (; it_task != _migration_tasks.end(); it_task++) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_migration_tasks is a map, so why not just find the task by key?
You can refer to https://www.cnblogs.com/xupeidong/p/11976671.html

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_migration_tasks is a map, so why not just find the task by key?
You can refer to https://www.cnblogs.com/xupeidong/p/11976671.html

OK. Thank you.

LOG(WARNING) << "tablet migrate failed. tablet_id=" << tablet_id
<< ", schema_hash=" << schema_hash << ", dest_disk=" << dest_disk
<< ", status:" << res;
status = Status::InternalError("migration task failed");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better to add res to the message in status, so that user can known what's wrong

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better to add res to the message in status, so that user can known what's wrong

That sounds reasonable.

morningman
morningman previously approved these changes Jan 14, 2021
Copy link
Contributor

@morningman morningman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@morningman morningman added the approved Indicates a PR has been approved by one committer. label Jan 14, 2021
Copy link
Contributor

@morningman morningman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@morningman morningman merged commit 99b22c9 into apache:master Jan 16, 2021
EmmyMiao87 pushed a commit to EmmyMiao87/incubator-doris that referenced this pull request Jan 26, 2021
…fferent disks (apache#5101)

Based on PR apache#4475, this patch add a new feature for single tablet migration between different disks by http.
Co-authored-by: weizuo <weizuo@xiaomi.com>
@yangzhg yangzhg mentioned this pull request Feb 9, 2021
@weizuo93 weizuo93 deleted the single_tablet_migration_interface branch March 9, 2022 07:03
levy5307 pushed a commit to levy5307/incubator-doris that referenced this pull request Nov 14, 2022
…fferent disks (apache#5101)

Based on PR apache#4475, this patch add a new feature for single tablet migration between different disks by http.
Co-authored-by: weizuo <weizuo@xiaomi.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api-review Categorizes an issue or PR as actively needing an API review. approved Indicates a PR has been approved by one committer. kind/feature Categorizes issue or PR as related to a new feature.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants