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 DiffFiles utility function #1374

Merged
merged 1 commit into from
Sep 27, 2023
Merged

Fix DiffFiles utility function #1374

merged 1 commit into from
Sep 27, 2023

Conversation

gargnitingoogle
Copy link
Collaborator

@gargnitingoogle gargnitingoogle commented Sep 13, 2023 •
edited
Loading

This rationalizes the memory requirement
of file-op utility DiffFiles. This utility
earlier used to the read the complete files
in memory, but now only a chunk is read and
compared at a time.
It also changes the name (to AreFilesIdentical) and the return-type of the utility to boolean (true=identical, false=otherwise) .

Description

Link to the issue in case of a bug fix.

NA

Testing details

  1. Manual - NA
  2. Unit tests - NA
  3. Integration tests - No new tests. Existing tests passed.

@gargnitingoogle gargnitingoogle added the execute-integration-tests Run only integration tests label Sep 13, 2023
@gargnitingoogle gargnitingoogle force-pushed the gargnitin-fix-difffiles branch 2 times, most recently from f9dba4a to 399be1e Compare September 13, 2023 12:55
@gargnitingoogle gargnitingoogle marked this pull request as ready for review September 13, 2023 12:57
@gargnitingoogle gargnitingoogle changed the title Chunkify DiffFile utility function Fix DiffFiles utility function Sep 14, 2023
@gargnitingoogle
Copy link
Collaborator Author

@vadlakondaswetha apologies, this PR wasn't ready for review/merge yet. I created it as draft, but that seemed to stop the integration test label from working. So I had temporarily switched to 'ready to review' . I'll notify you once it's ready for review.

@gargnitingoogle gargnitingoogle changed the title Fix DiffFiles utility function [Dont Review/Merge] Fix DiffFiles utility function Sep 14, 2023
@gargnitingoogle
Copy link
Collaborator Author

gargnitingoogle commented Sep 14, 2023 •
edited
Loading

@Tulsishah this PR has just the execute-integration-tests label, not the perf tests label, and integration tests didn't run for it. I changed it from draft to 'ready for review' but it still didn't run. Am I missing something?

@gargnitingoogle gargnitingoogle marked this pull request as draft September 14, 2023 03:53
@gargnitingoogle gargnitingoogle force-pushed the gargnitin-fix-difffiles branch 2 times, most recently from 2c121bf to e7bd962 Compare September 14, 2023 04:16
@Tulsishah
Copy link
Collaborator

@Tulsishah this PR has just the execute-integration-tests label, not the perf tests label, and integration tests didn't run for it. I changed it from draft to 'ready for review' but it still didn't run. Am I missing something?

I can see it is working. I can also see perf-test label with this PR. do you face any other problem?

@gargnitingoogle
Copy link
Collaborator Author

@Tulsishah this PR has just the execute-integration-tests label, not the perf tests label, and integration tests didn't run for it. I changed it from draft to 'ready for review' but it still didn't run. Am I missing something?

I can see it is working. I can also see perf-test label with this PR. do you face any other problem?

Oh yeah, they hadn't shown up earlier.

I can also see perf-test label with this PR

No, I didn't add perf-test label, and I don't see it enabled either.

do you face any other problem?

No, thanks @Tulsishah .

@Tulsishah Tulsishah added the execute-perf-test Execute performance test in PR label Sep 14, 2023
gargnitingoogle
gargnitingoogle commented Sep 14, 2023
View reviewed changes
Copy link
Collaborator Author

@gargnitingoogle gargnitingoogle left a comment

Choose a reason for hiding this comment

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

Added couple of minor self-comments.

tools/integration_tests/util/operations/file_operations.go Outdated Show resolved Hide resolved
tools/integration_tests/util/operations/file_operations.go Outdated Show resolved Hide resolved
@gargnitingoogle gargnitingoogle removed the execute-perf-test Execute performance test in PR label Sep 14, 2023
@gargnitingoogle gargnitingoogle force-pushed the gargnitin-fix-difffiles branch 2 times, most recently from bde5501 to bfe60c6 Compare September 15, 2023 04:38
@gargnitingoogle gargnitingoogle changed the title [Dont Review/Merge] Fix DiffFiles utility function Fix DiffFiles utility function Sep 15, 2023
@gargnitingoogle gargnitingoogle marked this pull request as ready for review September 15, 2023 04:42
@gargnitingoogle gargnitingoogle force-pushed the gargnitin-fix-difffiles branch 3 times, most recently from 479286b to 38bb843 Compare September 15, 2023 04:49
@gargnitingoogle gargnitingoogle requested review from Tulsishah and removed request for vadlakondaswetha September 25, 2023 12:10
@gargnitingoogle
Fix DiffFiles utility function
ce5e844 
1. It chunkifies DiffFiles utility function.
This rationalizes the memory requirement
of file-op of this utility. This utility
earlier used to the read the complete files
in memory, but now only a chunk is read and
compared at a time.

2. Rename DiffFiles to AreFilesIdentical

Renames file-op utility DiffFiles to AreFilesIdentical
and changes it to return bool instead of int.
@gargnitingoogle gargnitingoogle changed the base branch from master to oct_2023_release September 26, 2023 06:57
@gargnitingoogle gargnitingoogle merged commit a52285f into oct_2023_release Sep 27, 2023
8 checks passed
@gargnitingoogle gargnitingoogle deleted the gargnitin-fix-difffiles branch September 27, 2023 04:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vadlakondaswetha vadlakondaswetha vadlakondaswetha approved these changes

@sethiay sethiay Awaiting requested review from sethiay sethiay is a code owner

@raj-prince raj-prince Awaiting requested review from raj-prince raj-prince is a code owner

@Tulsishah Tulsishah Awaiting requested review from Tulsishah

Assignees
No one assigned
Labels
execute-integration-tests Run only integration tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gargnitingoogle @Tulsishah @vadlakondaswetha