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

Fixed bitwise inversion for session id #138

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

segabriel
Copy link
Member

Motivation:

For now, we apply, for bitwise inversion of session id, Integer.MAX_VALUE. And it equals to 0x7fffffff, that means that all bit will be inverted apart from sign bit. It's wrong and we should use 0xffffffff as a value or just bitwise inversion operator (~).

@artem-v
Copy link

artem-v commented Mar 14, 2019

apart from sign bit

Why you bother about sign of session id? We use reactor.aeron.SecureRandomSessionIdGenerator which guarantees session ids in range 0 .. 0x7fffffff .

Plus in AeronResources we hardcode this one:

          // explicit range of reserved session ids
          .publicationReservedSessionIdLow(0)
          .publicationReservedSessionIdHigh(Integer.MAX_VALUE);

@artem-v
Copy link

artem-v commented Mar 14, 2019

  • If you really want to move forward this bitwise thing, go ahead and remove from AeronResources those publicationReservedSessionIdLow and publicationReservedSessionIdHigh
    and make SecureRandomSessionIdGenerator emit full range of integers.

  • Alternative approach -- ban setting sessionIdGenerator from options and have a hardcoded bsession id allocation logic as described at Fixed bitwise inversion for session id #138 (comment)

Copy link

@artem-v artem-v left a comment

Choose a reason for hiding this comment

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

As we agreed consider either of two points from: #138 (comment)

@segabriel segabriel requested a review from artem-v March 14, 2019 13:46
@segabriel segabriel dismissed artem-v’s stale review March 14, 2019 13:47

I chose the first your approach

@skolomiiets skolomiiets added question Further information is requested benchmarks aeron-benchmark and removed benchmarks aeron-benchmark question Further information is requested labels Mar 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready For Review Ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants