NSQnet icon indicating copy to clipboard operation
NSQnet copied to clipboard

TLS/Deflate/Snappy; NSQnet compatibility guarantees

Open allgeek opened this issue 9 years ago • 2 comments

I'm working on some changes that I plan to submit a pull request for, which will add support for a number of new NSQ protocol options as well as negotiation for TLS/Deflate/Snappy support, and have a couple questions.

  1. Of these changes, the addition of Snappy may be most controversial, since it introduces dependencies on a native library & P/Invoke. I'm using https://www.nuget.org/packages/Snappy.NET/ which includes support for 32 & 64 bit environments. Any concerns/issues with adding this new reference (and the downstream dependency of https://www.nuget.org/packages/Crc32C.NET/)?
  2. Are there any strong versioning / compatibility guarantees for NSQClient? I'd like to introduce a NSQOptions class to house the more detailed configuration information, and am wondering how much I can/should clean up existing constructor overloads, or if I should preserve those (as well as passthrough getters/setters on the ShortIdentifier/LongIdentifier/HeartbeatMilliseconds properties, which would be duplicating the underlying options for the client). Given that NSQ has deprecated the ShortIdentifier & LongIdentifier names, a wholesale cleanup/update to match the current protocol state would be cleaner, but would be a breaking change. Does it make sense to roll a new major version for this, or would you prefer to have it implemented as a minor version with compatibility (marking the existing constructors/properties obsolete)? Taking advantage of the new options will require changes for anyone using NSQClient already, and the 'breaking changes' would be relatively minor...
  3. A potentially less problematic breaking change - any objection to marking the default NSQClient constructor as private instead of public (regardless of the major/minor versioning decisions)? As far as I can tell, that constructor is pointless, as there's no way to configure the nsqd hostname/port after construction (the NSQClient Hostname/Port properties are read-only) - which means the client should fail upon Initialize(). It seems making that constructor private would make usage more intuitive.

Thanks, Levi

allgeek avatar Mar 14 '15 03:03 allgeek

  1. I would live to avoid depending on native libararies if possible. This library should work in both windows and anywhere mono runs.
  2. For the sake of simplicity lets just target the most current NSQ version. I haven't had a ton of time to devote to updating the protocol, if you want to submit a PR for that stuff it would be super helpful.
  3. Seems fine to make the default ctor private / eliminate it.

wcharczuk avatar Mar 14 '15 16:03 wcharczuk

Thanks for the feedback - I totally understand the desire to keep Mono compatibility; that's why I figured I should ask. Unfortunately the only managed implementations of Snappy I've seen so far are incomplete and abandoned (and would be much slower, as the native implementations can leverage hardware-accelerated CRC32C).

I'll see if I can find a good way to make Snappy support with the native libraries an optional build target (#ifdef etc - not sure how to do optional NuGet dependencies yet), so the official build can still support Mono (but would not include Snappy support), but the Snappy code is in there for any Windows users desiring their own builds (or you can publish a separate Snappy-enabled Windows build/NuGet package if desired).

Otherwise I may submit a PR without any Snappy-related code (and may maintain a fork with Snappy if we decide we want to use it internally).

allgeek avatar Mar 14 '15 17:03 allgeek