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

server: Don't mute exceptions #98

Merged
merged 1 commit into from
Apr 12, 2023
Merged

server: Don't mute exceptions #98

merged 1 commit into from
Apr 12, 2023

Conversation

epuertat
Copy link
Member

@epuertat epuertat commented Apr 11, 2023 •
edited
Loading

Fixes: #97

From https://docs.python.org/3/reference/datamodel.html#object.__exit__:

object.__exit__(self, exc_type, exc_value, traceback)

(...)

If an exception is supplied, and the method wishes to suppress the exception
(i.e., prevent it from being propagated), it should return a true value.
Otherwise, the exception will be processed normally upon exit from this method.

@epuertat epuertat self-assigned this Apr 11, 2023
@epuertat epuertat added the bug Something isn't working label Apr 11, 2023
idryomov
idryomov reviewed Apr 11, 2023
View reviewed changes
control/server.py Outdated Show resolved Hide resolved
@idryomov
Copy link
Contributor

idryomov commented Apr 11, 2023 •
edited
Loading

fix(server): Don't mute exceptions

None of the existing commits use this (conventional commits?) style. If we wanted to adopt it, it's probably fine but this needs to be discussed and made explicit since nothing else in Ceph uses it today.

@idryomov idryomov added this to the #2 milestone Apr 11, 2023
@epuertat
Copy link
Member Author

fix(server): Don't mute exceptions

None of the existing commits use this (conventional commits?) style. If we wanted to adopt it, it's probably fine but this needs to be discussed and made explicit since nothing else in Ceph uses it today.

Agree @idryomov. I'll fix this and propose this topic for the next Tuesday (although it overlaps with Cephalocon).

My rationale here is that given we're a separate repo from ceph/ceph, I assume that we would keep a separate versioning scheme as well. And as we rely on a gRPC schema and 2 other integration pieces (SPDK and Ceph repos) that might require a thorough cross-dependency versioning.

Here I thought that conventional commits (and semantic versioning, which we're not following in ceph/ceph) might help manage release numbering and deal with breaking changes across the multiple repos involved. I'd also facilitate creating release notes/changelogs.

@idryomov
Copy link
Contributor

propose this topic for the next Tuesday (although it overlaps with Cephalocon).

Yeah, not sure if we are having a meeting next Tuesday...

@epuertat
server: Don't mute exceptions
8b55884 
Fixes: #97

Signed-off-by: Ernesto Puerta <epuertat@redhat.com>
@epuertat epuertat force-pushed the 97-errors-are-no-longer-reported branch from 66f0dc9 to 8b55884 Compare April 12, 2023 11:45
@epuertat epuertat changed the title fix(server): Don't mute exceptions server: Don't mute exceptions Apr 12, 2023
@epuertat
Copy link
Member Author

@idryomov I addressed both comments.

@epuertat epuertat requested a review from idryomov April 12, 2023 11:48
@idryomov idryomov merged commit de73461 into devel Apr 12, 2023
@idryomov idryomov deleted the 97-errors-are-no-longer-reported branch April 12, 2023 12:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@idryomov idryomov idryomov approved these changes

@sskaur sskaur Awaiting requested review from sskaur

Assignees

@epuertat epuertat

Labels
bug Something isn't working
Projects
NVMe-oF
Archived in project
Milestone
#2 - Reef Feature Freeze
Development

Successfully merging this pull request may close these issues.

Errors are no longer reported
2 participants
@epuertat @idryomov