Skip to content
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

Switch to protobuf #23

Merged
merged 7 commits into from
Jul 26, 2021
Merged

Switch to protobuf #23

merged 7 commits into from
Jul 26, 2021

Conversation

lujiajing1126
Copy link
Contributor

@lujiajing1126 lujiajing1126 commented Jul 25, 2021

WARNING: Huge amount of change has been made in this PR.

  • Flatbuffers fully removed. (I guess there is still some references in go.sum)
  • Protbuf introduced with adaptions made to fit with pb conventions.
  • (Local) Tests Pass for pkg, banyand directories.
  • License Check. (proto files are not supported by Skywalking-eyes, manually added. Skip *.pb.go and *.textproto)
  • Use buf to lint and generate protos.

Problem found in this PR

Relative timestamp in ChunkID

The timestamp contained in chunkID is not the same as the timestamp field in the entity since we are using a relative time shift. This shift will cause inaccurate Scan results.

Signed-off-by: Megrez Lu <lujiajing1126@gmail.com>
Signed-off-by: Megrez Lu <lujiajing1126@gmail.com>
Signed-off-by: Megrez Lu <lujiajing1126@gmail.com>
Signed-off-by: Megrez Lu <lujiajing1126@gmail.com>
Copy link
Contributor

@hanahmily hanahmily left a comment

Choose a reason for hiding this comment

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

it looks good for now. We should improve the codes to pass the CI check. And I recommend leveraging buf to manage proto files.

generate-go:
Copy link
Contributor

Choose a reason for hiding this comment

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

Would we leverage https://github.com/bufbuild/buf to check proto files and generate stubs?

Copy link
Contributor Author

@lujiajing1126 lujiajing1126 Jul 25, 2021

Choose a reason for hiding this comment

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

I've added buf.yaml and buf.gen.yaml to leverage buf.

I exclude two rules, i.e. RPC_REQUEST_STANDARD_NAME and RPC_RESPONSE_STANDARD_NAME since they require *Request and *Response for request and response types. Do we need to impose these two rules?

banyand/series/trace/write.go Outdated Show resolved Hide resolved
@lujiajing1126
Copy link
Contributor Author

@hanahmily I need some help from you to setup CI workflow. It seems there is restriction of Actions usage in this repo.

Could you help setup the workflow?

@hanahmily
Copy link
Contributor

@hanahmily I need some help from you to setup CI workflow. It seems there is restriction of Actions usage in this repo.

Could you help setup the workflow?

How about leave the workflow alone, let's merge it with some CI failures, I will submit another PR to fix them

Signed-off-by: Megrez Lu <lujiajing1126@gmail.com>
Signed-off-by: Megrez Lu <lujiajing1126@gmail.com>
Signed-off-by: Megrez Lu <lujiajing1126@gmail.com>
@lujiajing1126
Copy link
Contributor Author

@hanahmily I need some help from you to setup CI workflow. It seems there is restriction of Actions usage in this repo.
Could you help setup the workflow?

How about leave the workflow alone, let's merge it with some CI failures, I will submit another PR to fix them

Sure.

@hanahmily hanahmily merged commit 98ad1f8 into apache:main Jul 26, 2021
@lujiajing1126 lujiajing1126 deleted the protobuf branch July 26, 2021 03:48
@lujiajing1126 lujiajing1126 mentioned this pull request Jul 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants