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

feat: Improve progress tracking #2888

Merged
merged 7 commits into from
May 21, 2024
Merged

feat: Improve progress tracking #2888

merged 7 commits into from
May 21, 2024

Conversation

e-sumin
Copy link
Contributor

@e-sumin e-sumin commented May 16, 2024

Change Overview

Currently we are tracking only amounts of data uploaded during action or phase. This PR brings new counters which allows us to track both, uploaded and downloaded data.

Pull request type

Please check the type of change your PR introduces:

  • 🚧 Work in Progress
  • 🌈 Refactoring (no functional changes, no api changes)
  • 🐹 Trivial/Minor
  • 🐛 Bugfix
  • 🌻 Feature
  • 🗺️ Documentation
  • 🤖 Test

Issues

  • fixes #issue-number

Test Plan

  • 💪 Manual
  • ⚡ Unit test
  • 💚 E2E

@e-sumin e-sumin changed the title Improve progress tracking feat: Improve progress tracking May 16, 2024
Copy link
Contributor

@viveksinghggits viveksinghggits left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -325,6 +341,22 @@ spec:
type: string
description: Progress of completion in percentage
jsonPath: .status.progress.percentCompleted
- name: SizeDownloadedB
type: integer
description: Amount of data downloaded during action execution
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
description: Amount of data downloaded during action execution
description: Amount of data that has been downloaded

what do you think about this?

Copy link
Contributor

Choose a reason for hiding this comment

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

if you agree, we can change other columns as well.

@@ -144,6 +144,26 @@ func completedOrFailed(aIDX int, actionSet *crv1alpha1.ActionSet, phaseName stri
return false
}

func isPhaseProgressDiffer(a, b crv1alpha1.PhaseProgress) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not able to figure out what exactly this function is, by reading the name. is it trying to figure out if phase progress is different?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

a and b both are phase progress, the function returns true if they are different, and false if they are similar.

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't think of a better name, do you have any ideas?

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe having different instead of differ would be a good idea? Or if we want to shorten we can just use Diff instead of differ.

@e-sumin e-sumin requested a review from viveksinghggits May 20, 2024 04:41
Copy link
Contributor

@viveksinghggits viveksinghggits left a comment

Choose a reason for hiding this comment

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

LGTM

pkg/apis/cr/v1alpha1/types.go Outdated Show resolved Hide resolved
@@ -144,6 +144,26 @@ func completedOrFailed(aIDX int, actionSet *crv1alpha1.ActionSet, phaseName stri
return false
}

func isPhaseProgressDiffer(a, b crv1alpha1.PhaseProgress) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe having different instead of differ would be a good idea? Or if we want to shorten we can just use Diff instead of differ.

e-sumin and others added 2 commits May 20, 2024 20:43
Co-authored-by: Vivek Singh <vivek.singh@veeam.com>
@e-sumin e-sumin requested a review from viveksinghggits May 20, 2024 18:45
@e-sumin
Copy link
Contributor Author

e-sumin commented May 20, 2024

All notes were accepted.

@@ -144,6 +144,26 @@ func completedOrFailed(aIDX int, actionSet *crv1alpha1.ActionSet, phaseName stri
return false
}

func isPhaseProgressDifferent(a, b crv1alpha1.PhaseProgress) bool {
Copy link
Contributor

@viveksinghggits viveksinghggits May 21, 2024

Choose a reason for hiding this comment

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

just one small nit please feel free to ignore it if you think otherwise. because we have two PhaseProgresses here, should we rename this to
arePhaseProgressesDifferent?

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 thought about this yesterday but my conclusion was - probably it's not so important. But since you have the same thoughts, probably that makes sense. Renamed.

@e-sumin e-sumin added the kueue label May 21, 2024
@mergify mergify bot merged commit ef161d0 into master May 21, 2024
15 checks passed
@mergify mergify bot deleted the improve_progress_tracking branch May 21, 2024 12:58
Copy link

@Setyania Setyania left a comment

Choose a reason for hiding this comment

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

Location

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants