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

Refactor: remvoe Copy bound from NodeId #1255

Merged
merged 1 commit into from
Oct 14, 2024

Conversation

drmingdrmer
Copy link
Member

@drmingdrmer drmingdrmer commented Oct 12, 2024

I hereby agree to the terms of the CLA available at: https://docs.databend.com/dev/policies/cla/

Summary

Refactor: remvoe Copy bound from NodeId

The NodeId type is currently defined as:

type NodeId: .. + Copy + .. + 'static;

This commit removes the Copy bound from NodeId.
This modification will allow the use of non-Copy types as NodeId,
providing greater flexibility for applications that prefer
variable-length strings or other non-Copy types for node
identification.

This change maintain compatibility by updating derived Copy
implementations with manual implementations:

// Before
#[derive(Copy...)]
pub struct LogId<NID: NodeId> {}

// After
impl<NID: Copy> Copy for LogId<NID> {}

This change is Reviewable

@drmingdrmer drmingdrmer force-pushed the 159-nid-no-copy branch 4 times, most recently from 740010d to ed3b446 Compare October 13, 2024 02:39
@drmingdrmer drmingdrmer marked this pull request as ready for review October 13, 2024 02:52
@drmingdrmer drmingdrmer requested a review from SteveLauC October 13, 2024 02:53
Copy link
Collaborator

@SteveLauC SteveLauC left a comment

Choose a reason for hiding this comment

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

Sorry for reviewing this PR on GitHub, reviewable does not show line numbers, which makes me impossible to locate the code and do a check in my IDE, left some comments.

BTW, there are multiple "unused" warnings reported by GitHub Actions on the review page, are they false-positive? If not, perhaps we can do a clean-up:)

openraft/src/core/raft_core.rs Outdated Show resolved Hide resolved
openraft/src/raft_state/mod.rs Outdated Show resolved Hide resolved
openraft/src/replication/mod.rs Outdated Show resolved Hide resolved
openraft/src/replication/mod.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@schreter schreter left a comment

Choose a reason for hiding this comment

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

@SteveLauC (no review from my side, just a tip)

reviewable does not show line numbers, which makes me impossible to locate the code and do a check in my IDE, left some comments

Actually, it does. It is just a bit cumbersome, since you have to set it up, see here. Similarly, for locating the code in the IDE, you can directly jump into the IDE from Reviewable from any comment (also from a new draft). It is documented here. With that, you don't even strictly need line numbers (BTW, those are shown in the draft of the new comment as well), but I personally did configure them for myself :-).

Reviewable status: 0 of 57 files reviewed, 4 unresolved discussions (waiting on @drmingdrmer and @SteveLauC)

@SteveLauC
Copy link
Collaborator

@SteveLauC (no review from my side, just a tip)

Amazing! TIL! Will set them up. 😄

The `NodeId` type is currently defined as:

```rust
type NodeId: .. + Copy + .. + 'static;
```

This commit removes the `Copy` bound from `NodeId`.
This modification will allow the use of non-`Copy` types as `NodeId`,
providing greater flexibility for applications that prefer
variable-length strings or other non-`Copy` types for node
identification.

This change maintain compatibility by updating derived `Copy`
implementations with manual implementations:

```rust
// Before
#[derive(Copy...)]
pub struct LogId<NID: NodeId> {}

// After
impl<NID: Copy> Copy for LogId<NID> {}
```

- Fix: databendlabs#1250
Copy link
Collaborator

@SteveLauC SteveLauC left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 57 files reviewed, 4 unresolved discussions (waiting on @drmingdrmer)

@drmingdrmer
Copy link
Member Author

@SteveLauC
Thank you for addressing these details. Your attention to these nuances is greatly appreciated.

@schreter
The Reviewable hints are indeed very helpful!

Unfortunately, I haven't figured out how to open CLion directly from Reviewable yet. My browser can open VS Code using the vscode:// protocol, but it doesn't work with clion:// for CLion.

I've learned that I need to manually register a URL handler for CLion. I'll try to set this up later. It's a bit disappointing that it's not as straightforward as with VS Code. :(

@drmingdrmer drmingdrmer merged commit 77d40a2 into databendlabs:main Oct 14, 2024
31 of 32 checks passed
@drmingdrmer drmingdrmer deleted the 159-nid-no-copy branch October 14, 2024 05:10
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.

Remove Copy Bound from NodeId
3 participants