Remove package cycles involving lease, internal, exception, and frame #774
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Move ClientSetup and ServerSetup are closely associated with the RSocketFactory implementations. Moving them into core where they are used from breaks a package cycle between resume and internal. ClientSetup is straight-forward and folds easily into DefaultClientRSocketFactory. ConnectionUtil is closely associated with ServerSetup and has been folded into it. Signed-off-by: Rossen Stoyanchev <[email protected]>
spencergibb
approved these changes
Apr 9, 2020
OlegDokuka
reviewed
Apr 10, 2020
rsocket-core/src/main/java/io/rsocket/frame/ErrorFrameFlyweight.java
Outdated
Show resolved
Hide resolved
OlegDokuka
reviewed
Apr 10, 2020
MissingLeaseException is moved to the lease package next to the contracts it is declared in breaking a cycle between lease and exceptions. This is an API breakage but I the impact should be minor (none? the chances of something handling it explicitly) and hence an acceptable trade-off. Signed-off-by: Rossen Stoyanchev <[email protected]>
37b779d to
34f6852
Compare
34f6852 to
1c2c3b6
Compare
Replaces (now deprecated) RSocketException from the exceptions package which can lead to cycles (e.g. with frame package). Other minor refinements: - error code validation - shared errorCode() implementation - error code in toString() - avoid NPE for null message, it's better to keep the original exception and null is allowed by getMessage(). Signed-off-by: Rossen Stoyanchev <[email protected]>
1c2c3b6 to
c686d5e
Compare
OlegDokuka
reviewed
Apr 10, 2020
| */ | ||
| public RSocketException(String message, @Nullable Throwable cause) { | ||
| super(Objects.requireNonNull(message, "message must not be null"), cause); | ||
| super(0x00000201, message, cause); |
Member
There was a problem hiding this comment.
can we use ErrorType.APPLICATION_ERROR here. In any case, we have ref to ErrorType
Suggested change
| super(0x00000201, message, cause); | |
| super(ErrorType.APPLICATION_ERROR, message, cause); |
Contributor
Author
There was a problem hiding this comment.
Yes, indeed, this should be a constant. I've suggested a further change to use the constants from ErrorFrameFlyweight.
OlegDokuka
reviewed
Apr 10, 2020
|
|
||
| import java.util.Objects; | ||
| import io.rsocket.RSocketErrorException; | ||
| import reactor.util.annotation.Nullable; |
Member
There was a problem hiding this comment.
Suggested change
| import reactor.util.annotation.Nullable; | |
| import io.rsocket.frame.ErrorType; | |
| import reactor.util.annotation.Nullable; |
OlegDokuka
approved these changes
Apr 10, 2020
Member
OlegDokuka
left a comment
There was a problem hiding this comment.
In general LGTM. Just a single minor proposal to use constant
This change deprecates ErrorType as the same constants are already declared in ErrorFrameFlyweight and there is hierarchy of exceptions with one exception per error code. Signed-off-by: Rossen Stoyanchev <[email protected]>
a16d367 to
fa16b55
Compare
OlegDokuka
approved these changes
Apr 11, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This resolves package cycles between:
lease-internaldue toClientSetupandServerSetup(both related to leasing).exception-leasedue to references betweenMissingLeaseExceptionandLease.exception-framedue to references betweenErrorFrameFlyweightandRSocketException.This is mostly backwards compatible and functionality neutral except for moving
MissingLeasingExceptiontolease. It's unlikely to break anything and a worthwhile trade-off.