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

Expose sequence recovery data via SessionStateListener #315

Merged
merged 4 commits into from
Oct 19, 2020

Conversation

sheinbergon
Copy link
Contributor

@sheinbergon sheinbergon commented Sep 22, 2020

Fixes #313

See #313 for additional details

@chrjohn
Copy link
Member

chrjohn commented Sep 22, 2020

Thanks for the PR!
I think this enhancement is sensible to keep the application free from logic to check for ResendRequest/SequenceReset messages in the toAdmin/fromAdmin callbacks.
We built logic to check whether our FIX process was currently waiting for a Resend to be satisfied. This could be largely simplified once this is implemented.

Edit: put to 3.0.0 milestone due to interface changes.

@chrjohn chrjohn added this to the QFJ 3.0.0 milestone Sep 22, 2020
@sheinbergon
Copy link
Contributor Author

Thanks for the PR!
I think this enhancement is sensible to keep the application free from logic to check for ResendRequest/SequenceReset messages in the toAdmin/fromAdmin callbacks.
We built logic to check whether our FIX process was currently waiting for a Resend to be satisfied. This could be largely simplified once this is implemented.

Yes, actuallty that was our exact use case. I tried rolling our own logic, saw it felt and acted 'kind of iffy' and decided to solve the problem at the source instead

Edit: put to 3.0.0 milestone due to interface changes.

I'm not well aware of QFJ's release cycle/policy, but I do see that a version is released once a year or so.
My only concern is that 3.0.0 means a release ETA of 2 years. While I have already built our own version and stored on our private nexus server, this is of course less than optimal.

@chrjohn Is it crucial that patch versions don't contain interface changes?

@chrjohn
Copy link
Member

chrjohn commented Sep 23, 2020

Edit: put to 3.0.0 milestone due to interface changes.

I'm not well aware of QFJ's release cycle/policy, but I do see that a version is released once a year or so.
My only concern is that 3.0.0 means a release ETA of 2 years. While I have already built our own version and stored on our private nexus server, this is of course less than optimal.

@chrjohn Is it crucial that patch versions don't contain interface changes?

We try to follow semantic versioning. We need to increase the major version when backward incompatible changes are introduced. Since you have added new interface methods, users need to adapt (i.e. add implementations for these methods) and recompile their applications. IMHO these are backward incompatible changes. But maybe I am misinterpreting this. @the-thing or @philipwhiuk , you have any opinion on that?

You could of course add default implementations in the interface which would avoid recompilation. However, this looks a bit ugly in the code when default and non-default methods are mixed in the interface.

Copy link
Contributor

@the-thing the-thing left a comment

Choose a reason for hiding this comment

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

I just had a quick look. The change itself looks OK. This is an enhancement, so it probably does not require immediate release.

I'm not against releasing it with the next minor with default methods. They have been used in the past quickfix.Connector, quickfix.MessageFactory etc.

@chrjohn
Copy link
Member

chrjohn commented Sep 23, 2020

OK, thanks for your opinion on this @the-thing . So how about converting all methods in SessionStateListener to default methods then? People already implementing these methods shouldn't care and all others are not forced to implement the (new) methods. Plus the interface does not mix both method types.

@sheinbergon
Copy link
Contributor Author

OK, thanks for your opinion on this @the-thing . So how about converting all methods in SessionStateListener to default methods then? People already implementing these methods shouldn't care and all others are not forced to implement the (new) methods. Plus the interface does not mix both method types.

@chrjohn Done, all interface methods now have void defaults.

@chrjohn chrjohn changed the title SessionStateListener now has new recovery awareness methods/endpoints Expose sequence recovery data via SessionStateListener Sep 24, 2020
@chrjohn chrjohn modified the milestones: QFJ 3.0.0, QFJ 2.2.1 Sep 24, 2020
@sheinbergon
Copy link
Contributor Author

@chrjohn Just checking. As the milestone has changed to QFJ 2.2.1, is there anything preventing us from merging this PR?

@chrjohn
Copy link
Member

chrjohn commented Oct 15, 2020

Time constraints... ;) Just wanted to take a closer look, hopefully the next days...

@chrjohn
Copy link
Member

chrjohn commented Oct 19, 2020

Just realized that @the-thing already took the time to do a review, so this is good to go.

@chrjohn chrjohn merged commit f577b30 into quickfix-j:master Oct 19, 2020
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.

Expose sequence recovery data externally via the SessionStateListener interface
3 participants