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

update import_sstpb and fix go & rust docker image build #1224

Merged
merged 11 commits into from
Mar 21, 2024
Merged

Conversation

lance6716
Copy link
Contributor

@lance6716 lance6716 commented Feb 20, 2024

ref tikv/tikv#16533

this PR still has C++ make entry error

$ ./scripts/docker-run.sh make c++ 
mkdir -p /home/lance/Projects/kvproto/bin
/home/lance/Projects/kvproto/scripts/check.sh
go: modules disabled by GO111MODULE=off; see 'go help modules'
mkdir -p kvprotobuild && cd kvprotobuild && cmake ../cpp -DCMAKE_PREFIX_PATH=$GRPC_INSTALL_PATH && make
-- Using gRPC:  : /grpcinstall/v1_26_0/include, /grpcinstall/v1_26_0/lib/libgrpc.a;/grpcinstall/v1_26_0/lib/libgrpc++.a;/grpcinstall/v1_26_0/lib/libcares.a, /grpcinstall/v1_26_0/bin/grpc_cpp_plugin
CMake Error at cmake/find_abseil-cpp.cmake:1 (find_package):
  By not providing "Findabsl.cmake" in CMAKE_MODULE_PATH this project has
  asked CMake to find a package configuration file provided by "absl", but
  CMake did not find one.

  Could not find a package configuration file provided by "absl" with any of
  the following names:

    abslConfig.cmake
    absl-config.cmake

  Add the installation prefix of "absl" to CMAKE_PREFIX_PATH or set
  "absl_DIR" to a directory containing one of the above files.  If "absl"
  provides a separate development package or SDK, be sure it has been
  installed.
Call Stack (most recent call first):
  CMakeLists.txt:26 (include)


-- Configuring incomplete, errors occurred!
See also "/home/lance/Projects/kvproto/kvprotobuild/CMakeFiles/CMakeOutput.log".
See also "/home/lance/Projects/kvproto/kvprotobuild/CMakeFiles/CMakeError.log".
make: *** [Makefile:27: c++] Error 1

I don't know how to fix it.

@@ -283,6 +283,7 @@ message Pair {
message WriteBatch {
uint64 commit_ts = 1;
repeated Pair pairs = 2;
uint64 start_ts = 3;
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you add some comments about this field? I guess the start_ts is more like a hint that needed to be provided while uploading a SST in write CF.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

(Also I guess such field should also be stored in SstMeta somehow. Because the observer can only get information about the SST in SstMeta.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can move start_ts into SstMeta, do you think it's enough? just to avoid the two places have different values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Signed-off-by: lance6716 <[email protected]>
Copy link
Contributor

@YuJuncen YuJuncen left a comment

Choose a reason for hiding this comment

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

Rest LGTM

@@ -283,6 +283,7 @@ message Pair {
message WriteBatch {
uint64 commit_ts = 1;
repeated Pair pairs = 2;
uint64 start_ts = 3;
Copy link
Contributor

Choose a reason for hiding this comment

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

(Also I guess such field should also be stored in SstMeta somehow. Because the observer can only get information about the SST in SstMeta.)

scripts/check.sh Outdated
@@ -20,8 +20,7 @@ check-protos-compatible() {
export PATH=$GOPATH/bin:$PATH

if [ ! -f "$GOPATH/bin/protolock" ]; then
GO111MODULE=off go get github.com/nilslice/protolock/cmd/protolock
GO111MODULE=off go install github.com/nilslice/protolock/cmd/protolock
GO111MODULE=off go install github.com/nilslice/protolock/cmd/protolock@latest
Copy link
Member

Choose a reason for hiding this comment

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

Please specify protolock version to keep proto.lock consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Signed-off-by: lance6716 <[email protected]>
Signed-off-by: lance6716 <[email protected]>
Copy link
Member

@overvenus overvenus left a comment

Choose a reason for hiding this comment

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

To avoid making unnecessary changes to kvproto, let's hold this PR until everything is working as expected.

YuJuncen and others added 3 commits February 23, 2024 16:11
commit c5325a8
Author: hillium <[email protected]>
Date:   Tue Feb 20 18:38:01 2024 +0800

    added more comments

    Signed-off-by: hillium <[email protected]>

commit e6198f3
Author: hillium <[email protected]>
Date:   Mon Feb 19 16:17:21 2024 +0800

    added file format for pitr

    Signed-off-by: hillium <[email protected]>

Signed-off-by: hillium <[email protected]>
Signed-off-by: lance6716 <[email protected]>
proto/brpb.proto Outdated
@@ -644,6 +644,11 @@ enum FileType {
Put = 1;
}

enum FileFormat {
KvStream = 0;
Copy link
Member

@overvenus overvenus Feb 26, 2024

Choose a reason for hiding this comment

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

enum tag 0 should be unknown or unspecified in proto3.

Please follow Proto Best Practices, thanks! https://protobuf.dev/programming-guides/dos-donts/#unspecified-enum

@disksing disksing merged commit 3fa61fc into master Mar 21, 2024
4 checks passed
@disksing disksing deleted the start-ts branch March 21, 2024 06:26
lance6716 added a commit that referenced this pull request Mar 21, 2024
lance6716 added a commit that referenced this pull request Mar 21, 2024
)

* Revert "update import_sstpb and fix go & rust docker image build (#1224)"

This reverts commit 3fa61fc.

* some scripts don't need to be reverted

Signed-off-by: lance6716 <[email protected]>

---------

Signed-off-by: lance6716 <[email protected]>
@wuhuizuo
Copy link
Contributor

wuhuizuo commented Apr 1, 2024

the CI is broken:
image

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.

5 participants