Skip to content

StreamIdSupplier returns 1,3,5... or 2,4,6... #213

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

Merged
merged 3 commits into from
Jan 3, 2017
Merged

StreamIdSupplier returns 1,3,5... or 2,4,6... #213

merged 3 commits into from
Jan 3, 2017

Conversation

yschimke
Copy link
Member

No description provided.

@NiteshKant
Copy link
Contributor

@yschimke I replied with my thoughts on issue #212

I think semantically we should stick to isValid() doing what it does today but initializing the supplier with -1 and 0 makes sense. Can you revert the isValid() change?

Also, I think this class should be pkg-private and isValid() should perhaps be named isBeforeCurrent() (or some better name)? If you want you can make those changes too 😄

Copy link
Contributor

@NiteshKant NiteshKant left a comment

Choose a reason for hiding this comment

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

Thanks @yschimke ! I will pull this in once you address my comments.

@yschimke
Copy link
Member Author

yschimke commented Jan 3, 2017

So I don't think I should fully revert the isValid changes, since I still think its flipped logically. For an unknown stream id, I still think you only want to log when the stream id is negative or greater than the last issued.

Also the class is used outside the package so I can't make the other change without refactoring.

@NiteshKant
Copy link
Contributor

Also the class is used outside the package so I can't make the other change without refactoring.

Duh, I didn't notice the outside package usages, sorry about that.

Thanks @yschimke !

@NiteshKant NiteshKant merged commit 272401b into rsocket:0.5.x Jan 3, 2017
@yschimke yschimke deleted the stream_ids branch May 6, 2017 06:58
ilayaperumalg pushed a commit to ilayaperumalg/rsocket-java that referenced this pull request Dec 26, 2017
Solve SETUP_ERROR confusion: rsocket/rsocket#191
Exactly specify what error codes are applicable any time an ERROR frame is referenced.
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.

2 participants