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

Added MasterClientID setting #213

Merged
merged 4 commits into from
May 28, 2023
Merged

Added MasterClientID setting #213

merged 4 commits into from
May 28, 2023

Conversation

blades
Copy link
Contributor

@blades blades commented May 15, 2023

I know this particular issue was raised a couple of years ago, but no PR was provided at the time and it was mostly a discussion about whether or not there was a use-case for this similar to the use-case for inclusion of the port number setting. Similar to the port setting, it won't change an existing value if the flag isn't set in the configuration file.

I've chosen to create this on my own fork as I've needed to have this setting available for provision during both unit testing and in production. Investigation of other options for this setting have led me to believe that it's not possible to set this through any configuration file (whether human-editable or otherwise) for inclusion in a unit testing scenario or for automatic docker builds where this value needs to be set. I was only able to find the setting in an xml settings export as an attribute against an element, and it's not stored anywhere in the file tree once set. If I've missed something obvious, I'd be happy to be proven wrong!

My specific use case for this is that I have code that requires this value to be set in order to obtain real-time trade information as it happens from the API. Without this set, only trades made with the same client ID can be accessed, which potentially means that we'd miss trades. Setting this value means that all trades on the gateway are available, and that's the only way to meet our requirement.

For unit testing (which makes extensive use of containers), I need to be able to set this value to test that the code is correctly obtaining all trades when it's set, or only some trades when it isn't. In production, I also want to be able to set this cleanly within the dockerised production environment without having to manually adjust values because the system essentially recreates a dockerised gateway from scratch in the event of any sort of issue (or when the gateway is updated). Relying on someone to manually add that value whenever a gateway is re-instantiated isn't viable when there are multiple gateways in use.

TL;DR version: I needed it, couldn't find any other way to do it, so made my own fork to make it happen. I've created a PR but I appreciate it's a configuration setting.

@rlktradewright
Copy link
Member

Thanks for your very comprehensive statement. I haven't had time to look at the commits yet, but will do so soon.

@rlktradewright
Copy link
Member

Ok, I've looked through this now and have a few comments that need to be addressed before I will merge it.

  1. Don't include IBC.jar in the commit. IBC.jar is built and committed separately as part of the release process.

  2. Change the new OverrideMasterClientID setting in config.ini to OverrideTwsMasterClientID, for consistency with the existing OverrideTwsApiPort setting. Note also that the header comment for the new setting needs an additional '-' character in its underline.

  3. The change to IbcTws.java is in conflict with the current version. Please update to use a similar structure which takes account of the need to check for running the FIX gateway (this was necessary because the FIX gateway doesn't use the API configuration dlialogue).

Thanks.

@blades
Copy link
Contributor Author

blades commented May 22, 2023

I believe that I've addressed those points above now. Thanks for the very clear direction on those, by the way - very much appreciated!

@rlktradewright rlktradewright merged commit af1199c into IbcAlpha:master May 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants