tiflow icon indicating copy to clipboard operation
tiflow copied to clipboard

engine: rename the enginepb package name to pb

Open lonng opened this issue 3 years ago • 10 comments

Signed-off-by: Henry Lonng [email protected]

What problem does this PR solve?

Issue Number: ref #6356

What is changed and how it works?

The package github.com/pingcap/tiflow/engine/enginepb has a duplicated word engine, and it's better to keep the same package naming style with github.com/pingcap/tiflow/dm/pb because all imports use pb as its alias (see below picture).

3vqs59YDFr

This PR refactors the package name of github.com/pingcap/tiflow/engine/enginepb and eliminate the duplicated word engine.

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code

Questions

Will it cause performance regression or break compatibility?
Do you need to update user documentation, design documentation or monitoring documentation?

Release note

None

lonng avatar Aug 04 '22 08:08 lonng

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • amyangfei
  • gozssky

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment. After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review. Reviewer can cancel approval by submitting a request changes review.

ti-chi-bot avatar Aug 04 '22 08:08 ti-chi-bot

I will address the conflicts after two reviewers LGTM.

lonng avatar Aug 04 '22 09:08 lonng

@lonng: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

ti-chi-bot avatar Aug 04 '22 09:08 ti-chi-bot

@lonng @amyangfei The reason why engine change pkg name from 'pb' to 'enginepb' is : https://github.com/pingcap/tiflow/issues/5555 https://github.com/pingcap/tiflow/pull/5627

I think we need to do more work.

maxshuang avatar Aug 04 '22 09:08 maxshuang

/hold

maxshuang avatar Aug 04 '22 09:08 maxshuang

@lonng @amyangfei The reason why engine change pkg name from 'pb' to 'enginepb' is : #5555 #5627

I think we need to do more work.

Yes, I forget this issue. What about using the structure in pingcap/kvproto, https://github.com/pingcap/kvproto/tree/master/pkg, such as

pb
├── cdc
├── dm
└── engine

But we need package alias when using them.

amyangfei avatar Aug 04 '22 09:08 amyangfei

@lonng @amyangfei The reason why engine change pkg name from 'pb' to 'enginepb' is : #5555 #5627 I think we need to do more work.

Yes, I forget this issue. What about using the structure in pingcap/kvproto, https://github.com/pingcap/kvproto/tree/master/pkg, such as

pb
├── cdc
├── dm
└── engine

But we need package alias when using them.

Make sense, I will reorg the structure.

lonng avatar Aug 05 '22 00:08 lonng

@amyangfei Why not change the enginepb.WorkerInfo to pb.ExecutorInfo,

message WorkerInfo {
    string id = 1;
    string executor_id = 2;
    bytes  status = 3;
    bytes  config = 4;
    bool  is_tombstone = 5;
    int64  last_hb_time = 6;
    int64  workload = 7;
}

What the difference between id and executor_id?

lonng avatar Aug 05 '22 01:08 lonng

@amyangfei Why not change the enginepb.WorkerInfo to pb.ExecutorInfo,

message WorkerInfo {
    string id = 1;
    string executor_id = 2;
    bytes  status = 3;
    bytes  config = 4;
    bool  is_tombstone = 5;
    int64  last_hb_time = 6;
    int64  workload = 7;
}

What the difference between id and executor_id?

enginepb.WorkerInfo here is the information of a worker in master-worker framework, where worker can be treated as a runtime task and runs real job such as DM task. So id is the worker id, executor_id means on which executor is the worker running. @lonng

amyangfei avatar Aug 05 '22 04:08 amyangfei

@amyangfei Got it.

lonng avatar Aug 05 '22 05:08 lonng

https://github.com/pingcap/tiflow/pull/6669 has changed the proto inlcude directory from engine/proto to project directory. So I guess there will be no pb type collision like pb.WorkerInfo.

Now engine is using the official golang protobuf. Golang protobuf doesn't allow duplicate filename globally, see https://github.com/golang/protobuf/issues/1122. That's why I changed the include directory. Otherwise, it will conflict with https://github.com/pingcap/tipb.

To avoid such problems completely, I think we can refer to https://github.com/googleapis/googleapis structure. Not only the generated codes but also the proto files, need to have a unique import path. So that we don't need to change our message name, file name or package name, which looks weird. There is a simple example in https://github.com/juanfont/headscale/tree/main/proto/headscale/v1.

sleepymole avatar Aug 22 '22 05:08 sleepymole

This PR needs further work to merge and I will close it.

lonng avatar Sep 06 '22 08:09 lonng