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

Wip gc get source dest zones #2

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

Conversation

AlvinYangZF
Copy link

Add two functions to find GC source zones and GC dest zones.

Copy link
Collaborator

@skyzh skyzh left a comment

Choose a reason for hiding this comment

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

While I have no background on this project, I simply gave some suggestion based on my understanding. Point me out if I'm wrong.

for(auto ext_it : existFile->extents_) {

ZoneExtent* extent;
extent = ext_it;
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems that extent and ext_it are the same thing. Should we use a single identifier for both of them?

Choose a reason for hiding this comment

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

The code may not be the latest. I did some modifications to it.

Zone* zone_idx = extent->zone_;
//only care about the FULL zone.
if(!zone_idx->IsFull()) {
break;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I have no background about this project, but I doubt this logic. If one zone of an extent is already full, shall we ignore all following extents? If I guess correctly, we should use continue here.

Choose a reason for hiding this comment

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

It is my code, continue is correct so far,.
Zifeng would help to fix it.

@AlvinYangZF AlvinYangZF force-pushed the wip_gc_get_source_dest_zones branch from a9b593e to b15721d Compare July 5, 2021 13:07
typo of get source zones for GC
set allocate zone for GC as no life time class
clang-format-11 the code style = file
@AlvinYangZF AlvinYangZF force-pushed the wip_gc_get_source_dest_zones branch from b901bde to 37f8637 Compare July 6, 2021 13:03
@skyzh
Copy link
Collaborator

skyzh commented Jul 7, 2021

The patch is too big for a PR. It contains many more style changes than actual changes. I guess you have run "clang-format" with a different config, and run it again with current config.

I suggest reverting latest commit in your branch and re-format it, so as to reduce size of this patch.

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

Successfully merging this pull request may close these issues.

3 participants