dynamic initiator session (QFJ-247) #157

andyflury · 2017-11-24T17:22:19Z

QFJ supports dynamic acceptor sessions, however the same does not exist for initiator sessions. This pull request would allow applications to createDynamic initiator sessions through AbstractSocketInitiator.createDynamicSession

Also, I added a new session level boolean setting "Inactive", that can be added to settings that should not start automatically when starting QuickFix. So this session can be started on demand with above new method. The constant SETTING_INACTIVE_SESSION would probably have to be moved somewhere else.

This might not be the proper way to do this, but it seems to be working.

Any comments are welcome

chrjohn · 2017-11-27T08:18:35Z

Hi @andyflury , thanks for the PR. Could you please document the intended usage of the new setting in https://github.com/quickfix-j/quickfixj/blob/master/quickfixj-core/src/main/doc/usermanual/usage/configuration.html ?

Thanks,
Chris.

chrjohn · 2017-11-27T08:21:26Z

Just found this old JIRA issue... :)
https://www.quickfixj.org/jira/browse/QFJ-247

andyflury · 2017-11-27T08:56:30Z

Hi Chris, thanks! I updated the documentation according to your suggestion

philipwhiuk · 2017-11-28T22:33:44Z

quickfixj-core/src/main/java/quickfix/mina/initiator/AbstractSocketInitiator.java

    protected final Logger log = LoggerFactory.getLogger(getClass());
    private final Set<IoSessionInitiator> initiators = new HashSet<>();
+
+    private static final String SETTING_INACTIVE_SESSION = "Inactive";


Belongs in Initiator class.

philipwhiuk

I would suggest this belongs in 2.1.0 given it's a feature not a bugfix.

Regarding the 'Inactive' flag - seems strange to have that for Dynamic Initiator and not the Dynamic Acceptor. Shouldn't they work similarly (SocketConnectHost vs SocketAcceptHost is already an annoyance to me)?

philipwhiuk · 2017-11-28T22:34:10Z

quickfixj-core/src/main/java/quickfix/mina/initiator/AbstractSocketInitiator.java

                try {
-                    final Session quickfixSession = createSession(sessionID);
-                    initiatorSessions.put(sessionID, quickfixSession);
+                    if (!settings.isSetting(sessionID, SETTING_INACTIVE_SESSION) || !settings.getBool(sessionID, SETTING_INACTIVE_SESSION)) {


Some tests on this would be nice.

philipwhiuk · 2017-11-28T22:35:03Z

quickfixj-core/src/main/java/quickfix/mina/initiator/AbstractSocketInitiator.java

        setSessions(initiatorSessions);
    }
+
+    public void createDynamicSession(SessionID sessionID) throws ConfigError {


Again for a new feature it would be nice to see tests. For a considerable feature like a dynamic session some of the end-to-end / regression tests seem appropriate.

andyflury · 2017-11-29T18:24:48Z

@philipwhiuk I'll be happy to provide a test case. Could you point me to an existing test case I could use as a basis for the new test case?

andyflury · 2017-11-29T18:29:21Z

I also moved the setting to the Initiator class, thanks for pointing this out

chrjohn · 2018-01-25T10:35:23Z

Hi @andyflury ,
there is a test in SessionConnectorTest and DynamicAcceptorSessionProviderTest. Maybe they can be used as a template?
If you need help, just ask.
Thanks,
Chris.

baboono · 2018-05-23T15:01:24Z

@philipwhiuk
I hope we are not too stuck here. I agree AbstractSocketInitiator.createDynamicSession would be very usefull. Is it possible if I provide end-to-end / regression test ? Can we refresh that issue a bit to keep it alive?

chrjohn · 2018-05-24T06:24:36Z

Hi,
I think any help is appreciated. :) Feel free to push some changes or open a separate PR.
Cheers,
Chris.

baboono · 2018-06-01T12:05:32Z

I can see test ready here. If this is sufficient enough for pull request it will be great.

chrjohn · 2018-06-03T21:26:50Z

Hi @andyflury ,
the test is failing:

[ERROR] Failures: 
[ERROR]   SessionConnectorTest.testDynamicInitiatorSession:237 expected:<2> but was:<4>

I think the initiators are only removed from the set if the connector is stopped. Otherwise you will end up with double the initiators.

andyflury · 2018-06-04T13:26:17Z

Hi @chrjohn Glad to get Your comment up here. From what I understand in this this part in AbstractSocketInitiator.class:
protected void createSessionInitiators()
throws ConfigError {
try {
// QFJ698: clear() is needed on restart, otherwise the set gets filled up with
// more and more initiators which are not equal because the local port differs
initiators.clear();
...
clear() should be removed from here and instead its should be placed in stopInitiators()?

chrjohn · 2018-06-04T13:41:38Z

Hi @andyflury ,

I think you have an outdated version of the code. ;)
That clear() statement has been removed here: #175 (beginning of March). And indeed now the initiators are cleaned up in stopInitiators().

Calling createSessionInitiators() does not check if the old initiators have been stopped. This is only done when calling from SocketInitiator.stop().

So I guess in your test you could just stop the connector and then re-create the sessions.

Corrected the assertion and added one more to check , if initiators are removed.

andyflury · 2018-06-04T16:32:13Z

No magic here I guess. Would that be sufficient?
//This should remove initiators
connector.stopInitiators();
assertEquals(0,connector.getInitiators().size());
//Tear down
for(Session s:sessions){
s.close();
}

chrjohn · 2018-06-04T17:47:03Z

That should do the trick, thanks.
I restarted the build, there was another (unrelated) error.

baboono · 2018-06-05T09:38:41Z

Hi @chrjohn
Is it possible to know , when are You going to do release with this feature?

andyflury · 2018-06-05T11:53:57Z

Thank You @chrjohn . Is there an option to merge the changes?

chrjohn · 2018-06-06T09:02:40Z

Hi guys,
I corrected the name of the setting in the documentation (changed from Inactive to DynamicSession).
Apart from that everything is looking fine to me and I am going to merge the changes.
I'll probably open a PR to unify the setting with dynamic acceptors (there it is called AcceptorTemplate at the moment, but DynamicSession could be used for both now).
Regarding the release: there are some minor things I want to incorporate into the release, but I'll try to be finished with that by the end of the month.

Cheers,
Chris.

andyflury · 2018-06-06T11:02:18Z

Great @chrjohn . Thanks for all Your support. I hope we ll get a change to talk again :)

chrjohn · 2018-06-06T11:27:33Z

You're welcome. Thanks for the pull request!
Yes, I guess there will be a chance. :)
Cheers!

create dynamic initiator session

48dd318

chrjohn changed the title ~~dynamic initiator session~~ dynamic initiator session (QFJ-247) Nov 27, 2017

chrjohn added this to the QFJ 2.0.1 milestone Nov 27, 2017

update documentation

4a3ad00

minimal whitespace changes

cac34e2

philipwhiuk reviewed Nov 28, 2017

View reviewed changes

move and rename SETTING_DYNAMIC_SESSION

e6c9228

chrjohn modified the milestones: QFJ 2.0.1, QFJ 2.1.0 Jan 25, 2018

chrjohn added the TODO label Jan 25, 2018

Added dynamic initiator sessions Test

e2f728f

chrjohn added 2 commits June 3, 2018 22:33

Merge branch 'master' into dynamic-initiator-sessions

4379e7a

re-add import which was accidentally removed

aa88bc2

Added content to the test

723e729

Corrected the assertion and added one more to check , if initiators are removed.

correct name of setting in documentation

316dd24

chrjohn removed the TODO label Jun 6, 2018

chrjohn merged commit c9b51d9 into quickfix-j:master Jun 6, 2018

dynamic initiator session (QFJ-247) #157

dynamic initiator session (QFJ-247) #157

Uh oh!

Conversation

andyflury commented Nov 24, 2017

Uh oh!

chrjohn commented Nov 27, 2017

Uh oh!

chrjohn commented Nov 27, 2017

Uh oh!

andyflury commented Nov 27, 2017

Uh oh!

philipwhiuk Nov 28, 2017

Choose a reason for hiding this comment

Uh oh!

philipwhiuk left a comment

Choose a reason for hiding this comment

Uh oh!

philipwhiuk Nov 28, 2017

Choose a reason for hiding this comment

Uh oh!

philipwhiuk Nov 28, 2017

Choose a reason for hiding this comment

Uh oh!

andyflury commented Nov 29, 2017

Uh oh!

andyflury commented Nov 29, 2017

Uh oh!

chrjohn commented Jan 25, 2018

Uh oh!

baboono commented May 23, 2018

Uh oh!

chrjohn commented May 24, 2018

Uh oh!

baboono commented Jun 1, 2018

Uh oh!

chrjohn commented Jun 3, 2018

Uh oh!

andyflury commented Jun 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrjohn commented Jun 4, 2018

Uh oh!

andyflury commented Jun 4, 2018

Uh oh!

chrjohn commented Jun 4, 2018

Uh oh!

baboono commented Jun 5, 2018

Uh oh!

andyflury commented Jun 5, 2018

Uh oh!

chrjohn commented Jun 6, 2018

Uh oh!

andyflury commented Jun 6, 2018

Uh oh!

chrjohn commented Jun 6, 2018

Uh oh!

Uh oh!

andyflury commented Jun 4, 2018 •

edited

Loading