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

enhance: API integration with storage v2 in clustering-compactions #40133

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

enhance: API integration with storage v2 in clustering-compactions #40133

wants to merge 6 commits into from

Conversation

tedxu
Copy link
Contributor

@tedxu tedxu commented Feb 24, 2025 •
edited
Loading

See #39173

@sre-ci-robot sre-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines. label Feb 24, 2025
@sre-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tedxu

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@mergify mergify bot added dco-passed DCO check passed. kind/enhancement Issues or changes related to enhancement labels Feb 24, 2025
@mergify Mergify
Copy link
Contributor

mergify bot commented Feb 24, 2025

@tedxu Please associate the related issue to the body of your Pull Request. (eg. “issue: #”)

@dosubot dosubot bot added the area/api label Feb 24, 2025
@mergify Mergify
Copy link
Contributor

mergify bot commented Feb 24, 2025

@tedxu E2e jenkins job failed, comment /run-cpu-e2e can trigger the job again.

@codecov Codecov
Copy link

codecov bot commented Feb 24, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 75.00000% with 64 lines in your changes missing coverage. Please review.

Project coverage is 80.56%. Comparing base (84df80b) to head (82a8640).
Report is 49 commits behind head on master.

Files with missing lines Patch % Lines
...ternal/datanode/compaction/clustering_compactor.go 65.64% 38 Missing and 7 partials ⚠️
internal/datanode/compaction/segment_writer.go 69.56% 13 Missing and 1 partial ⚠️
internal/datanode/compaction/mix_compactor.go 76.92% 2 Missing and 1 partial ⚠️
internal/storage/serde.go 92.85% 1 Missing ⚠️
internal/storage/stats.go 0.00% 1 Missing ⚠️

❌ Your patch check has failed because the patch coverage (75.00%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #40133      +/-   ##
==========================================
+ Coverage   80.55%   80.56%   +0.01%     
==========================================
  Files        1470     1473       +3     
  Lines      206087   205962     -125     
==========================================
- Hits       166014   165937      -77     
+ Misses      34058    34034      -24     
+ Partials     6015     5991      -24
Components Coverage Δ
Client 79.25% <ø> (ø)
Core 69.85% <ø> (-0.03%) ⬇️
Go 82.36% <75.00%> (+0.01%) ⬆️
Files with missing lines Coverage Δ
internal/datacoord/compaction_task_clustering.go 79.24% <100.00%> (-0.13%) ⬇️
internal/datacoord/compaction_trigger_v2.go 71.88% <ø> (ø)
internal/datanode/compaction/compactor_common.go 58.37% <100.00%> (-10.32%) ⬇️
internal/datanode/compaction/l0_compactor.go 89.06% <100.00%> (-0.04%) ⬇️
internal/datanode/compaction/merge_sort.go 74.48% <100.00%> (ø)
internal/flushcommon/syncmgr/storage_serializer.go 48.22% <100.00%> (ø)
internal/querynodev2/segments/segment_loader.go 70.69% <100.00%> (-0.03%) ⬇️
internal/storage/serde_events.go 76.42% <100.00%> (+1.67%) ⬆️
internal/storage/serde_events_v2.go 60.80% <100.00%> (ø)
internal/util/importutilv2/binlog/l0_reader.go 84.74% <100.00%> (-0.26%) ⬇️
... and 5 more

... and 58 files with indirect coverage changes

@mergify Mergify
Copy link
Contributor

mergify bot commented Feb 24, 2025

@tedxu E2e jenkins job failed, comment /run-cpu-e2e can trigger the job again.

@mergify Mergify
Copy link
Contributor

mergify bot commented Feb 24, 2025

@tedxu cpp-unit-test check failed, comment rerun cpp-unit-test can trigger the job again.

@tedxu tedxu force-pushed the enhance/storage_v2_clustering_compaction branch from d19f445 to 79d88d2 Compare February 25, 2025 07:35
@mergify Mergify
Copy link
Contributor

mergify bot commented Feb 25, 2025

@tedxu go-sdk check failed, comment rerun go-sdk can trigger the job again.

@mergify Mergify
Copy link
Contributor

mergify bot commented Feb 25, 2025

@tedxu E2e jenkins job failed, comment /run-cpu-e2e can trigger the job again.

@mergify Mergify
Copy link
Contributor

mergify bot commented Feb 26, 2025

@tedxu E2e jenkins job failed, comment /run-cpu-e2e can trigger the job again.

@mergify Mergify
Copy link
Contributor

mergify bot commented Feb 27, 2025

@tedxu E2e jenkins job failed, comment /run-cpu-e2e can trigger the job again.

@tedxu
Copy link
Contributor Author

tedxu commented Feb 27, 2025

/run-cpu-e2e

@mergify mergify bot added the ci-passed label Feb 27, 2025
czs007
czs007 reviewed Mar 2, 2025
View reviewed changes
internal/datanode/compaction/segment_writer.go
}

// DONOT return an empty list if every insert of the segment is deleted,
// append an empty segment instead
Copy link
Collaborator

Choose a reason for hiding this comment

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

comments is not match with the method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will fix, thanks!

czs007
czs007 reviewed Mar 2, 2025
View reviewed changes
internal/datacoord/compaction_task_clustering.go
@@ -106,7 +106,6 @@ func (t *clusteringCompactionTask) Process() bool {
if currentState != lastState {
ts := time.Now().Unix()
lastStateDuration := ts - t.GetTaskProto().GetLastStateStartTime()
log.Info("clustering compaction task state changed", zap.String("lastState", lastState), zap.String("currentState", currentState), zap.Int64("elapse seconds", lastStateDuration))
Copy link
Collaborator

Choose a reason for hiding this comment

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

keep this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line is duplicated several lines later, which makes the log difficult to read.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@czs007 czs007 czs007 left review comments

@congqixia congqixia Awaiting requested review from congqixia

@sunby sunby Awaiting requested review from sunby

Assignees
No one assigned
Labels
approved area/api area/test ci-passed dco-passed DCO check passed. kind/enhancement Issues or changes related to enhancement sig/testing size/XXL Denotes a PR that changes 1000+ lines. test/integration integration test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tedxu @sre-ci-robot @czs007