Skip to content

Refactor shared storage. #98

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
Merged
merged 8 commits into from
Nov 17, 2020
Merged

Refactor shared storage. #98

merged 8 commits into from
Nov 17, 2020

Conversation

mathetake
Copy link
Contributor

@mathetake mathetake commented Nov 16, 2020 •
edited
Loading

refactor the shared storage code:

  • separate out SharedQueue functionalities from ShareData class since they are independent of each other. It looks like they do not need to even share mutex.
  • write unit tests on each class

In a sequel PR, I would tackle the leakage issue

@mathetake
Refactor: extract shared data logic to dedicated files
2684bc3 
Signed-off-by: mathetake <takeshi@tetrate.io>
@mathetake
Separate queue functionalities from kvs
c6cd50c 
Signed-off-by: mathetake <takeshi@tetrate.io>
@mathetake mathetake changed the title Shared data leak Resolve shared storage memory leak. Nov 16, 2020
@mathetake
Style: add missing Lisense, and rename
cc585c1 
Signed-off-by: mathetake <takeshi@tetrate.io>
PiotrSikora
PiotrSikora reviewed Nov 16, 2020
View reviewed changes
Copy link
Member

@PiotrSikora PiotrSikora left a comment

Choose a reason for hiding this comment

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

Could you avoid mixing big refactoring / moving code with bug fixes? While I don't mind the refactoring, it makes it impossible to review the fix.

@mathetake
Copy link
Contributor Author

right. Thanks. I will narrow down this PR scope only to refactoring and adding tests

@mathetake mathetake changed the title Resolve shared storage memory leak. Refactor shared storage. Nov 17, 2020
@mathetake
Add unittests for SharedData,Queue
04940d2 
Signed-off-by: mathetake <takeshi@tetrate.io>
@mathetake mathetake mentioned this pull request Nov 17, 2020
Closed
@mathetake mathetake marked this pull request as ready for review November 17, 2020 02:41
@mathetake
Copy link
Contributor Author

@PiotrSikora updated the PR description and title, and made ready for review! PTAL

mathetake
mathetake commented Nov 17, 2020
View reviewed changes
@mathetake mathetake requested a review from lizan November 17, 2020 03:37
@PiotrSikora
Copy link
Member

Thanks! Since, as you noted, they are completely independent, should we split them into src/shared_data.* and src/shared_queue.*? or do you prefer to leave them together?

@mathetake
Copy link
Contributor Author

should we split them into src/shared_data.* and src/shared_queue.*?

I will split them! Thanks for the comment

@mathetake
Review: have seprated files for SharedData and SharedQueue
39f8f7d 
Signed-off-by: mathetake <takeshi@tetrate.io>
@mathetake
Fix typo in comment
39ca8bd 
Signed-off-by: mathetake <takeshi@tetrate.io>
@mathetake
Add unittest for SharedData concurrent read/write
78fe739 
Signed-off-by: mathetake <takeshi@tetrate.io>
@mathetake
Add unittest for SharedQueue concurrent read/write
53facbe 
Signed-off-by: mathetake <takeshi@tetrate.io>
@PiotrSikora
Copy link
Member

This is just split + tests, right? No code changes?

@mathetake
Copy link
Contributor Author

YES, no code changes

@PiotrSikora PiotrSikora merged commit 44516a7 into proxy-wasm:master Nov 17, 2020
@PiotrSikora
Copy link
Member

Thanks!

@mathetake mathetake deleted the shared-data-leak branch November 17, 2020 08:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PiotrSikora PiotrSikora PiotrSikora approved these changes

@lizan lizan Awaiting requested review from lizan

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
@mathetake @PiotrSikora