Skip to content

Conversation

@guptask
Copy link
Collaborator

@guptask guptask commented Nov 7, 2021

This change is Reviewable

@guptask guptask self-assigned this Nov 7, 2021
Copy link

@igchor igchor left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @guptask and @victoria-mcgrath)


cachelib/shm/ShmCommon.cpp, line 166 at r1 (raw file):

    util::throwSystemError(errno, "invalid fd");
  }
  return fd;

Why did you remove the switch for errno? I think the behavior might be different after this change.

@guptask guptask force-pushed the segment_common_migration branch 3 times, most recently from bd400cd to 60d92e5 Compare November 9, 2021 19:03
Copy link
Collaborator Author

@guptask guptask left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @igchor and @victoria-mcgrath)


cachelib/shm/ShmCommon.cpp, line 166 at r1 (raw file):

Previously, igchor (Igor Chorążewicz) wrote…

Why did you remove the switch for errno? I think the behavior might be different after this change.

Reverted to the errno switch case handling

Copy link

@igchor igchor left a comment

Choose a reason for hiding this comment

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

:lgtm:, but I would like to wait with merging this until #6 and #2 (which contains some fixes for tests) is merged. it would be good to verify if all the tests are passing on CI for this change.

Reviewable status: 0 of 4 files reviewed, all discussions resolved (waiting on @victoria-mcgrath)

@guptask guptask force-pushed the segment_common_migration branch from ec3940a to 312524b Compare November 18, 2021 08:02
Copy link

@igchor igchor left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 7 files reviewed, 2 unresolved discussions (waiting on @guptask and @victoria-mcgrath)


cachelib/shm/FileShmSegment.cpp, line 219 at r4 (raw file):

int FileShmSegment::createNewSegment(const std::string& name) {
  constexpr static int createFlags = O_RDWR | O_CREAT | O_EXCL;

why you removed constexpr static?


cachelib/shm/ShmCommon.h, line 47 at r4 (raw file):

};

// TODO(SHM_FILE): maybe we could use this inside the Tier Config class?

this should be removed

@guptask guptask force-pushed the segment_common_migration branch from b1e197f to a0d4a7c Compare November 18, 2021 17:44
Copy link
Collaborator

@victoria-mcgrath victoria-mcgrath left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 4 files at r2, 3 of 4 files at r4, 3 of 3 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @guptask)

Copy link

@igchor igchor left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @guptask)

@igchor igchor merged commit 6975593 into pmem:develop Nov 19, 2021
guptask added a commit to guptask/CacheLib that referenced this pull request May 23, 2023
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