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

Cls issue 70 #101

Closed
wants to merge 3 commits into from
Closed

Cls issue 70 #101

wants to merge 3 commits into from

Conversation

crossjam
Copy link

Attempting to handle issue #70 by building on @jonesetc close patch in issue #100 to fold in a CLS message back to the nsqd.

  • Adds a cls message
  • Adds send_cls method to async conn
  • Adds close method to the Client class, now you can close Publishers too
  • Subsequently tweaks the Reader class

Close stuff happens in a lot of places, probably missed something.

I need to figure out how to roll in some tests as well.

@mreiferson
Copy link
Member

@crossjam thanks for taking a stab at this!

except Exception, e:
logger.warning('[%s:%s] error sending CLS, closing',
conn.id, self.name)
conn.close()
Copy link
Member

Choose a reason for hiding this comment

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

we need to add accounting for outstanding messages and wait until they're all responded to before we actually close the connection (after sending CLS)

Copy link
Author

Choose a reason for hiding this comment

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

Is this outstanding message accounting an issue for just Writers? Readers? Or both?

Also, is an outstanding count good enough, or do individual messages need to be tracked?

Copy link
Member

Choose a reason for hiding this comment

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

just the reader - a count is sufficient.

Copy link
Author

Choose a reason for hiding this comment

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

How about I boost this up to the conn level, where there's already a counter for in-flight messages?

Also, what do you think about tacking on a time-out where one gives up on any outstanding messages and just sends the CLS?

Copy link
Member

Choose a reason for hiding this comment

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

SGTM

@mreiferson
Copy link
Member

closing this, it's been 6 years :) (issue #70 is still open tracking this)

@mreiferson mreiferson closed this Jun 14, 2020
@crossjam
Copy link
Author

No worries. It was about time ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants