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.

Sign up for GitHub

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

Jump to bottom

feat: add DfdaemonUpload.sync_host. #929

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

feat: add DfdaemonUpload.sync_host. #929

wants to merge 6 commits into from

Conversation

baowj-678
Copy link
Contributor

@baowj-678 baowj-678 commented Jan 6, 2025 •
edited
Loading

Description

  • Implement DfdaemonUpload.sync_host to respond for SyncHost requests.

Related Issue

dragonflyoss/dragonfly#3713

Motivation and Context

Screenshots (if appropriate)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation Update (if none of the other choices apply)
    Checklist
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.

@baowj-678 baowj-678 requested a review from a team as a code owner January 6, 2025 02:18
@codecov Codecov
Copy link

codecov bot commented Jan 6, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 0% with 102 lines in your changes missing coverage. Please review.

Project coverage is 32.33%. Comparing base (81ff733) to head (66c2d3e).
Report is 19 commits behind head on main.

Files with missing lines Patch % Lines
dragonfly-client/src/grpc/dfdaemon_upload.rs 0.00% 102 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #929      +/-   ##
==========================================
- Coverage   34.58%   32.33%   -2.25%     
==========================================
  Files          60       60              
  Lines        9308     9892     +584     
==========================================
- Hits         3219     3199      -20     
- Misses       6089     6693     +604
Files with missing lines Coverage Δ
dragonfly-client/src/grpc/dfdaemon_upload.rs 0.00% <0.00%> (ø)

... and 20 files with indirect coverage changes

gaius-qi
gaius-qi requested changes Jan 7, 2025
View reviewed changes
Copy link
Member

@gaius-qi gaius-qi left a comment

Choose a reason for hiding this comment

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

Add parent selector to resource.

gaius-qi
gaius-qi requested changes Jan 8, 2025
View reviewed changes
dragonfly-client/src/grpc/dfdaemon_upload.rs Outdated
@@ -176,6 +183,9 @@ pub struct DfdaemonUploadServerHandler {

/// persistent_cache_task is the persistent cache task manager.
persistent_cache_task: Arc<persistent_cache_task::PersistentCacheTask>,

/// sync_interval is the interval to send host info by sync_host.
sync_interval: Duration,
Copy link
Member

Choose a reason for hiding this comment

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

Remove sync_interval.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sync_host requires the parameter sync_interval to control the refresh interval. Can this parameter be included in the SyncHostRequest later on.

gaius-qi
gaius-qi requested changes Jan 8, 2025
View reviewed changes
Copy link
Member

@gaius-qi gaius-qi left a comment

Choose a reason for hiding this comment

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

Remove tokio::spawn.

gaius-qi
gaius-qi requested changes Jan 9, 2025
View reviewed changes
Copy link
Member

@gaius-qi gaius-qi left a comment

Choose a reason for hiding this comment

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

There are many problems. Continue to optimize the code.

@baowj-678 baowj-678 changed the title feat: add HostInfo and DfdaemonUpload.sync_host. feat: add DfdaemonUpload.sync_host. Jan 23, 2025
gaius-qi
gaius-qi requested changes Jan 24, 2025
View reviewed changes
dragonfly-client/src/grpc/dfdaemon_upload.rs
) -> Result<Response<Self::SyncHostStream>, Status> {
unimplemented!()
/// DEFAULT_INTERFACE_SPEED is the default speed for interfaces.
const DEFAULT_INTERFACE_SPEED: ByteSize = ByteSize::mb(10000 / 8);
Copy link
Member

Choose a reason for hiding this comment

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

Remove.

dragonfly-client/src/grpc/dfdaemon_upload.rs
const DEFAULT_INTERFACE_SPEED: ByteSize = ByteSize::mb(10000 / 8);
/// FIRST_HOST_INFO_REFRESH_INTERVAL is the interval for the first refresh of the host info
/// when start up.
const FIRST_HOST_INFO_REFRESH_INTERVAL: Duration = Duration::from_millis(100);
Copy link
Member

Choose a reason for hiding this comment

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

Remove.

dragonfly-client/src/grpc/dfdaemon_upload.rs
/// when start up.
const FIRST_HOST_INFO_REFRESH_INTERVAL: Duration = Duration::from_millis(100);
/// DEFAULT_HOST_INFO_REFRESH_INTERVAL is the default interval for refreshing the host info.
const DEFAULT_HOST_INFO_REFRESH_INTERVAL: Duration = Duration::from_secs(1);
Copy link
Member

Choose a reason for hiding this comment

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

Change to 500ms.

dragonfly-client/src/grpc/dfdaemon_upload.rs
/// DEFAULT_HOST_INFO_REFRESH_INTERVAL is the default interval for refreshing the host info.
const DEFAULT_HOST_INFO_REFRESH_INTERVAL: Duration = Duration::from_secs(1);
/// MILLISECONDS_PER_SECOND is the number of milliseconds contained per second.
const MILLISECONDS_PER_SECOND: u64 = 1_000;
Copy link
Member

Choose a reason for hiding this comment

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

Remove.

dragonfly-client/src/grpc/dfdaemon_upload.rs
/// MILLISECONDS_PER_SECOND is the number of milliseconds contained per second.
const MILLISECONDS_PER_SECOND: u64 = 1_000;
/// BITS_PER_BYTE is the number of bits contained per byte.
const BITS_PER_BYTE: u64 = 8;
Copy link
Member

Choose a reason for hiding this comment

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

Remove.

@gaius-qi gaius-qi mentioned this pull request Jan 24, 2025
Open
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gaius-qi gaius-qi gaius-qi requested changes

@jim3ma jim3ma Awaiting requested review from jim3ma jim3ma is a code owner automatically assigned from dragonflyoss/dragonfly2-reviewers

@anjia0532 anjia0532 Awaiting requested review from anjia0532 anjia0532 is a code owner automatically assigned from dragonflyoss/dragonfly2-reviewers

@mfarooq-tech mfarooq-tech Awaiting requested review from mfarooq-tech mfarooq-tech is a code owner automatically assigned from dragonflyoss/dragonfly2-reviewers

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@baowj-678 @gaius-qi