Skip to content
This repository has been archived by the owner on Dec 3, 2019. It is now read-only.

Check supported server protocols during endpoint discovery #1

Merged
merged 1 commit into from
Mar 23, 2017

Conversation

venkat1109
Copy link
Contributor

Currently, the client does not check for the advertised protocols from server. Instead it makes an assumption about the streaming protocol and uses a hard-coded port number (for the streaming endpoint). This patch does the following:

  • Parses the supported protocols during discovery and separates out the streaming/rpc ports
  • Uses the streaming ports for data, rpc port for consumer side ack/nacks.

In addition to the change above, this patch also deletes the IntegrationTest file (which was previously ignored). This code is being refactored (WIP).

@venkat1109 venkat1109 self-assigned this Mar 22, 2017
@coveralls
Copy link

Coverage Status

Coverage increased (+0.4%) to 69.962% when pulling 3305233 on protocol into 0f14017 on master.

Copy link

@aravindvs aravindvs left a comment

Choose a reason for hiding this comment

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

Overall the changes look straightforward. Thanks for fixing. Just had a question on the integration test.

@@ -1,302 +0,0 @@
/*******************************************************************************

Choose a reason for hiding this comment

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

Is this removed because we no longer need it? or will it be added later once we have a proper cherami-server mock?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The existing integ tests are simply broken and they aren't currently run as part of the regular builds. And I have a work in progress that totally rewrites the integration test (along with a proper implementation of cherami-server mock). When that patch is out, this file will be removed anyways. Given that background, this patch simply deletes the file instead of attempting to fix the references within that file.

Choose a reason for hiding this comment

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

ok cool..

@venkat1109 venkat1109 merged commit bc145e5 into master Mar 23, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants