Fuller interface documentation

[benlangfeld]

Adds documentation for most of the public interface. In some cases, methods were exposed publicly (or as protected) for no reason, and so I reduced the surface area of the public interface somewhat.

Note: if #181 is merged first, this will need to be rebased and modified.

[jeremyevans]

The documentation changes look fine.

Unless we are going to bump the major version, I don't think we should be removing public API (especially Client#close). If we are planning to bump the major version, then it is fine as long as all public API removal is documented in the CHANGELOG. @SamSaffron are we planning to bump the major version for the next release?

Can you explain the reason for removing the spec from connection_manager_spec?

I'm not asking for this PR to be split, but it may have been better to have a PR just for documentation, and a separate PR for any behavior changes.

[benlangfeld]

@jeremyevans I'm working on the basis that since the API was never documented, semver concerns (bumping to the next major version) don't really apply. I've not modified any of the API that is really used by a message_bus consumer, only internals which consuming applications don't really care about.

The removal from connection_manager_spec.rb is because it was testing a method that was not being used anywhere.

I've been somewhat aggressive with trimming the public API because message_bus has accumulated a large API surface area somewhat by accident; the methods that I made private were not public for any reason.

[jeremyevans]

I don't think it's good to remove public API without bumping unless there really is no reason for the method to be called externally. That may be true for a large number of the public API removals made in this PR. However, if a user of message_bus is using Client#close instead of Client#cancel, I'm not sure why we would want to break their code.

It would be helpful to have a list of all public API removals (either by removing the method entirely or by making it private), as even if we don't bump the major, we would want to document that in the CHANGELOG. Such a list would also make it easier for reviewers to determine if any method should be kept public.

[benlangfeld]

Methods removed:

• ConnectionManager#stats (never used and the ConnectionManager is not exposed to application code)
• Client#close (effectively duplicate of Client#cancel and the Client is only available via the ConnectionManager, thus not available to application code)
• Not yet removed, but a candidate for the same reasons: ConnectionManager#lookup_client

Methods made private:

• MessageBus::Implementation#encode_channel_name
• MessageBus::Implementation#decode_channel_name
• Client#in_async?
• Client#ensure_closed!
• ConnectionManager#subscribe_client
• Diagnostics.full_process_path
• Diagnostics.hostname
• MessageBus::Rack::Diagnostics#js_asset
• MessageBus::Rack::Diagnostics#generate_script_tag
• MessageBus::Rack::Diagnostics#file_hash
• MessageBus::Rack::Diagnostics#asset_contents
• MessageBus::Rack::Diagnostics#asset_path
• MessageBus::Rack::Diagnostics#index
• MessageBus::Rack::Diagnostics#translate_handlebars
• MessageBus::Rack::Diagnostics#indent
• MessageBus::Rack::Middleware#start_listener
• MessageBus::Rack::Middleware#close_db_connection!
• MessageBus::Rack::Middleware#add_client_with_timeout

Methods switched from protected to private:

• MessageBus::Implementation#global?
• MessageBus::Implementation#decode_message!
• MessageBus::Implementation#replay_backlog
• MessageBus::Implementation#subscribe_impl
• MessageBus::Implementation#unsubscribe_impl
• MessageBus::Implementation#ensure_subscriber_thread
• MessageBus::Implementation#new_subscriber_thread
• MessageBus::Implementation#global_subscribe_thread
• MessageBus::Implementation#multi_each
• Client#write_headers
• Client#write_chunk
• Client#write_and_close
• Client#messages_to_json

[jeremyevans]

Thanks, that information is helpful and I agree with the public API removals. I'm OK with merging this, so it just needs to wait for Sam's review.

[SamSaffron]

Overall I am good with these changes ... the removal of #close from client though maybe is a bit odd naming wise. Maybe we throw away cancel instead and just keep close implemented as cancel, why would we want to close and keep the timer around, seems somewhat odd to say the least

Go wild with the diagnostics stuff, I prototyped it years ago and never had a chance to add polish it needed

Also very happy to reduce surface area. Can you do a quick review of: https://github.com/discourse/discourse/blob/master/config/initializers/004-message_bus.rb to ensure nothing will break, I doubt it will, the stuff you touched is unrelated.

Point release wise, I feel as soon as we have the redis stream ready we should push a version out there.

Given the huge investment you are making in documentation I would really love it if message bus had a home page on the web, linking to documentation, with examples of large consumers (like shopify / discourse :) ) and so on, something to think about.

Feel free to merge after you consider the cancel / close rename

[benlangfeld]

I switched usage of Client#cancel to Client#close, maintaining only the latter. I reviewed the discourse initializer and it doesn't use anything I removed.

I'm happy to do a landing page in the coming weeks once I've got some of the work on Redis Streams and Diagnostics out the way.