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

Fix(lsbackup): Fix profiler in lsBackup (#7729) #8432

Merged
merged 4 commits into from
Dec 1, 2022
Merged

Fix(lsbackup): Fix profiler in lsBackup (#7729) #8432

merged 4 commits into from
Dec 1, 2022

Conversation

dshekhar95
Copy link
Contributor

@dshekhar95 dshekhar95 commented Nov 14, 2022 •
edited by joshua-goldstein
Loading

cherry pick from #7729

Problem

wrong config set for profiler (set to Restore instead of Backup)<!--
Please add a description with these things:

  1. Explain the problem by providing a good description.
  2. If it fixes any GitHub issues, say "Fixes #GitHubIssue".
  3. If it corresponds to a Jira issue, say "Fixes DGRAPH-###".
  4. If this is a breaking change, please prefix [Breaking] in the title. In the description, please put a note with exactly who these changes are breaking for.
    -->

Solution

set profiler to Backup.conf

ahsanbarkati and others added 2 commits November 14, 2022 12:21
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ dshekhar95
❌ ahsanbarkati
You have signed the CLA already but the status is still pending? Let us recheck it.

@github-actions github-actions bot added the area/enterprise Related to proprietary features label Nov 14, 2022
@dshekhar95 dshekhar95 marked this pull request as ready for review November 14, 2022 20:39
@meghalims meghalims added cherry-pick-22.0.2 Label for all cherry-picks for 22.0.2 Release and removed cherry-pick labels Nov 14, 2022
@coveralls
Copy link

coveralls commented Nov 14, 2022 •
edited
Loading

Coverage Status

Coverage increased (+0.007%) to 37.19% when pulling 6ff354d on dshekhar95-cherrypick-7729-fix-isBackup into bc5f584 on main.

@dshekhar95 dshekhar95 changed the title Dshekhar95 cherrypick 7729 fix is backup fix (bug): LsBackup Nov 14, 2022
@dshekhar95 dshekhar95 changed the title fix (bug): LsBackup Fix(lsbackup): Fix profiler in lsBackup (#7729) Nov 15, 2022
@dshekhar95 dshekhar95 requested a review from meghalims November 15, 2022 22:22
meghalims
meghalims previously requested changes Nov 18, 2022
View reviewed changes
Copy link
Contributor

@meghalims meghalims left a comment

Choose a reason for hiding this comment

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

Hi Dilip, as we have discussed, please add unit test for backup/run.go.

akon-dey
akon-dey reviewed Nov 19, 2022
View reviewed changes
ee/backup/run.go Outdated
@@ -42,6 +43,7 @@ import (
)

// Restore is the sub-command used to restore a backup.

Copy link

Choose a reason for hiding this comment

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

This is an unnecessary change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@akon-dey the var Restore x.SubCommand on line 47 is used in other part of the code. So I had to re-introduce to fix the errors

Copy link
Contributor Author

Choose a reason for hiding this comment

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

looks like github isnt showing the line 47 addition unless you click view changes, not sure if you missed that

Copy link
Member

Choose a reason for hiding this comment

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

I think the problem Akon is referring to is the blank line between the comment and the variable declaration.

akon-dey
akon-dey reviewed Nov 19, 2022
View reviewed changes
ee/backup/run.go
@@ -162,7 +164,7 @@ func initBackupLs() {
Short: "List info on backups in a given location",
Args: cobra.NoArgs,
Run: func(cmd *cobra.Command, args []string) {
defer x.StartProfile(Restore.Conf).Stop()
defer x.StartProfile(LsBackup.Conf).Stop()
Copy link

Choose a reason for hiding this comment

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

LGTM

@dshekhar95
Copy link
Contributor Author

This will need an backup integration test

@mangalaman93 mangalaman93 self-requested a review November 26, 2022 19:12
@all-seeing-code
Copy link
Contributor

Please add some details about the change in the PR description.

dshekhar95
dshekhar95 commented Nov 30, 2022
View reviewed changes
Copy link
Contributor Author

@dshekhar95 dshekhar95 left a comment

Choose a reason for hiding this comment

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

fixed

@dshekhar95 dshekhar95 dismissed meghalims’s stale review November 30, 2022 17:33

this requires a backup to test

meghalims
meghalims approved these changes Nov 30, 2022
View reviewed changes
Copy link
Contributor

@meghalims meghalims left a comment

Choose a reason for hiding this comment

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

Looks good. Please merge

@dshekhar95 dshekhar95 merged commit 5ac56a1 into main Dec 1, 2022
@dshekhar95 dshekhar95 deleted the dshekhar95-cherrypick-7729-fix-isBackup branch December 1, 2022 16:33
@damonfeldman
Copy link

This fix allows profiling of the Backup operation to work properly if certain profilng flags are specified in the configuration. Previously, the system was profiling restore instead of backup.

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

@meghalims meghalims meghalims approved these changes

@matthewmcneely matthewmcneely matthewmcneely approved these changes

@skrdgraph skrdgraph Awaiting requested review from skrdgraph

@darkn3rd darkn3rd Awaiting requested review from darkn3rd

@mangalaman93 mangalaman93 Awaiting requested review from mangalaman93

@akon-dey akon-dey Awaiting requested review from akon-dey

Assignees
No one assigned
Labels
area/enterprise Related to proprietary features cherry-pick-22.0.2 Label for all cherry-picks for 22.0.2 Release
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@dshekhar95 @CLAassistant @coveralls @all-seeing-code @damonfeldman @akon-dey @matthewmcneely @meghalims @ahsanbarkati