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

Clean up the API by removing FIXConnection.getChannel() #250

Merged
merged 1 commit into from Jul 17, 2022

Conversation

ddimtirov
Copy link
Contributor

Removing getChannel() and introducing a second connection constructor with upstream and downstream pipe, allows us to get rid of virtually all generics and simplify the pipe code. It is also increases the encapsulation of the connection opening the possibility of alternative (non-channel based) transports for exotic scenarios, without affecting client code.

While the getChannel() may look like logical necessity, in practice it was not used in this codebase and I don't find usages in my codebase. If I wanted to get a handle of it, I could have gotten it at the place of constructing the connection.

Copy link
Contributor Author

@ddimtirov ddimtirov left a comment

Choose a reason for hiding this comment

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

Added some comments in the interesting spots - most/all of the changes in the other files are removing generics and imports.

@ddimtirov
Copy link
Contributor Author

@jvirtanen please and let me know if you approve of the idea - if yes, I'll fix the failing checks.

If no - please close :-)

@jvirtanen
Copy link
Member

Hi @ddimtirov! Sorry, I missed this earlier in the week; I will have a look. 🔍

Copy link
Member

@jvirtanen jvirtanen left a comment

Choose a reason for hiding this comment

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

Nice work, @ddimtirov! 🙌🏼

I feel that the challenges in adopting Philadelphia to work with other transports than SocketChannel strengthen how I'm feeling this library should ultimately work: just provide methods to parse and format complete FIX messages from and to various buffers, leaving the reading and writing of these buffers outside the scope of the library. This would also improve interoperability with libraries such as Aeron and Netty.

@ddimtirov
Copy link
Contributor Author

Addressed all feedback, please have a look at the close() method - it looks weird, but it is compact and clean.

Copy link
Member

@jvirtanen jvirtanen left a comment

Choose a reason for hiding this comment

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

Looking good, @ddimtirov! 🙌🏼 I just left a few more comments, and then, as last time, could you please squash the changes into a single commit? 🙂

@ddimtirov
Copy link
Contributor Author

Done, the split commits (easier to review) are available in https://github.com/ddimtirov/philadelphia/tree/philadelphia-250-maintenance

@jvirtanen jvirtanen merged commit 4c6358c into paritytrading:main Jul 17, 2022
@jvirtanen
Copy link
Member

Thank you, @ddimtirov! 🚀

@ddimtirov
Copy link
Contributor Author

@jvirtanen, what about a release? Is there anything that you would like done before that?

@jvirtanen
Copy link
Member

@ddimtirov Yes! I've started some preparations for the 2.0.0 release. 👍🏼

@ddimtirov
Copy link
Contributor Author

Let me know if you need any help (tests, measurements, doc drafts, etc.)

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

Successfully merging this pull request may close these issues.

None yet

2 participants