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

copr: Implement ChunkedVecEnum #8948

Merged
merged 11 commits into from
Nov 6, 2020
Merged

copr: Implement ChunkedVecEnum #8948

merged 11 commits into from
Nov 6, 2020

Conversation

Xuanwo
Copy link
Member

@Xuanwo Xuanwo commented Nov 3, 2020

What problem does this PR solve?

Implement ChunkedVecEnum

Check List

Tests

  • Unit test

Signed-off-by: Xuanwo <github@xuanwo.io>
Signed-off-by: Xuanwo <github@xuanwo.io>
@Xuanwo Xuanwo marked this pull request as ready for review November 3, 2020 06:48
@Xuanwo
Copy link
Member Author

Xuanwo commented Nov 3, 2020

Unittests will come in following commits

Copy link
Member

@skyzh skyzh 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

Signed-off-by: Xuanwo <github@xuanwo.io>
Signed-off-by: Xuanwo <github@xuanwo.io>
@Xuanwo Xuanwo requested a review from skyzh November 3, 2020 07:54
Signed-off-by: Xuanwo <github@xuanwo.io>
Signed-off-by: Xuanwo <github@xuanwo.io>
fn test_basics() {
let mut x = setup();
x.push(None);
x.push(Some(Enum::new(x.data.clone(), 2)));
Copy link
Member

Choose a reason for hiding this comment

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

I suggest adding support for directly pushing usize values to ChunkedEnumVec, so as to reduce overhead. We may work on this later.

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Nov 3, 2020
@skyzh skyzh requested a review from breezewish November 3, 2020 08:12

// FIXME: we need a set_data here, but for now, we set directly
let mut buf = BufferVec::new();
buf.push("我好强啊");
Copy link
Member

Choose a reason for hiding this comment

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

?

@ti-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Nov 4, 2020
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Nov 4, 2020
@breezewish
Copy link
Member

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Nov 4, 2020
@ti-srebot
Copy link
Contributor

Your auto merge job has been accepted, waiting for:

  • 8821

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@Xuanwo merge failed.

@Xuanwo
Copy link
Member Author

Xuanwo commented Nov 4, 2020

/run-all-tests

1 similar comment
@Xuanwo
Copy link
Member Author

Xuanwo commented Nov 4, 2020

/run-all-tests

@Xuanwo
Copy link
Member Author

Xuanwo commented Nov 4, 2020

/merge

@ti-srebot
Copy link
Contributor

@Xuanwo Oops! auto merge is restricted to Committers of the SIG.You are not a committer or co-leader or leader.

@skyzh
Copy link
Member

skyzh commented Nov 4, 2020

/merge

@andylokandy
Copy link
Contributor

/merge

1 similar comment
@andylokandy
Copy link
Contributor

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@Xuanwo merge failed.

@Xuanwo
Copy link
Member Author

Xuanwo commented Nov 5, 2020

/run-all-tests

@zhongzc
Copy link
Contributor

zhongzc commented Nov 5, 2020

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@Xuanwo merge failed.

@zhongzc
Copy link
Contributor

zhongzc commented Nov 6, 2020

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@Xuanwo merge failed.

@zhongzc
Copy link
Contributor

zhongzc commented Nov 6, 2020

[2020-11-06T01:11:52.651Z] [2020/11/06 09:02:27.943 +08:00] [FATAL] [lib.rs:481] ["called `Result::unwrap()` on an `Err` value: channel is full"] [backtrace="stack backtrace:\n   0: tikv_util::set_panic_hook::{{closure}}\n   1: std::panicking::rust_panic_with_hook\n             at library/std/src/panicking.rs:581\n   2: std::panicking::begin_panic_handler::{{closure}}\n             at library/std/src/panicking.rs:484\n   3: std::sys_common::backtrace::__rust_end_short_backtrace\n             at library/std/src/sys_common/backtrace.rs:141\n   4: rust_begin_unwind\n             at library/std/src/panicking.rs:483\n   5: core::panicking::panic_fmt\n             at library/core/src/panicking.rs:85\n   6: core::option::expect_none_failed\n             at library/core/src/option.rs:1238\n   7: <raftstore::coprocessor::region_info_accessor::RegionEventListener as raftstore::coprocessor::RoleObserver>::on_role_change\n   8: raftstore::store::peer::Peer<EK,ER>::handle_raft_ready_append\n   9: <raftstore::store::fsm::store::RaftPoller<EK,ER,T,C> as batch_system::batch::PollHandler<raftstore::store::fsm::peer::PeerFsm<EK,ER>,raftstore::store::fsm::store::StoreFsm<EK>>>::handle_normal\n  10: batch_system::batch::Poller<N,C,Handler>::poll\n  11: std::sys_common::backtrace::__rust_begin_short_backtrace\n  12: core::ops::function::FnOnce::call_once{{vtable.shim}}\n  13: <alloc::boxed::Box<F> as core::ops::function::FnOnce<A>>::call_once\n             at /rustc/b1496c6e606dd908dd651ac2cce89815e10d7fc5/library/alloc/src/boxed.rs:1042\n      <alloc::boxed::Box<F> as core::ops::function::FnOnce<A>>::call_once\n             at /rustc/b1496c6e606dd908dd651ac2cce89815e10d7fc5/library/alloc/src/boxed.rs:1042\n      std::sys::unix::thread::Thread::new::thread_start\n             at library/std/src/sys/unix/thread.rs:89\n  14: start_thread\n  15: __clone\n"] [location=components/raftstore/src/coprocessor/region_info_accessor.rs:152] [thread_name=raftstore-1-0]

CI failed due to this panic. Will fixed by #8971.

@Xuanwo
Copy link
Member Author

Xuanwo commented Nov 6, 2020

#8971 has been merged, let's rock!

@skyzh
Copy link
Member

skyzh commented Nov 6, 2020

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@Xuanwo merge failed.

@andylokandy
Copy link
Contributor

/run-all-tests

@zhongzc zhongzc added status/can-merge Indicates a PR has been approved by a committer. and removed status/can-merge Indicates a PR has been approved by a committer. labels Nov 6, 2020
@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot ti-srebot merged commit 308c641 into tikv:master Nov 6, 2020
@zhongzc
Copy link
Contributor

zhongzc commented Nov 6, 2020

I wonder why chunked_vec_enum isn't included in data_type/mod.rs?

I tried to add mod chunked_vec_enum; to data_type/mod.rs, then some compile failed messages were produced by the compiler.

@skyzh
Copy link
Member

skyzh commented Nov 6, 2020

I wonder why chunked_vec_enum isn't included in data_type/mod.rs?

I tried to add mod chunked_vec_enum; to data_type/mod.rs, then some compile failed messages were produced by the compiler.

This module is not used by any code in TiKV, so we may add it later. cc @Xuanwo would you please include this module together with ChunkedVecSet in #8988 ?

@Xuanwo
Copy link
Member Author

Xuanwo commented Nov 6, 2020

OK, I will give it a try.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants