esanchezros / quickfixj-spring-boot-starter

Spring Boot Starter for QuickFIX/J
Apache License 2.0
121 stars 56 forks source link

Create well-known properties under quickfixj.server.config #96

Open ggershaw opened 1 year ago

ggershaw commented 1 year ago

Currently, you need to provide the a string which has all default and session level settings in the quickfixj.server.configString property. This doesn't allow you to leverage boot's ability to override yaml properties and violates DRY.

You would need to provide the same value with slight differences for each env (dev, uat, prod)

In the below proposed approach, DRY is respected and we leverage boot's ability to override the sessions property.

The proposed properties are under ''' quickfixj: server: config: ''''

Proposed properties are:

  1. default property which is a string. The dev would put all the default settings here in 1 single string. This property should be populated in application.yaml and should not be overridden in application-uat.yml etc to respect DRY
  2. sessions property which is a list of the individual session level settings. This property would be overridden in application-uat.yml etc. Session level sessions tend to change for each env so this doesn't violate DRY. -- create well known properties for the typical session level quickfixj properties -- create a catch all map property named additionalSessionSettings for venue specific settings that we just add to the session and assume the dev knows what they are doing

Proposed example ''' quickfixj: server: config: default: defaultSettings sessions:

            name: -- used in code to identify the session's settings
            beginString: FIXT.1.1
            targetCompId: Venue1
            socketConnectHost: someip
            socketConnectPort: 9876
            additionalSessionSettings: 
                    setting1: value
                    setting2: value2

        -
            name: venue2
            beginString: FIXT.1.1
            targetCompId: VenueCompId
            socketConnectHost: someip
            socketConnectPort: 9876


---------------------------------
Additionally,

Initiator or Acceptor is determined by 
quickfixj:
  client:
vs.
quickfixj:
  server:

Don't need it in the configString. Violates DRY and could cause bug/confusion if they have diff values.
    ConnectionType=initiator

-----------------------------

Finally, I believe quickfixjserver property should be renamed quickfixj unless this property is only for the server. I'm not clear on that.

management:
  health:
    quickfixjserver:
      enabled: true
esanchezros commented 1 year ago

Hi @ggershaw, thanks for your suggestions. I think they are valid and probably the next logical step in improving the starter.

I myself had this same problem, when loading the settings file was only possible via classpath or absolute path. I thought about creating a replica of all quickfixj config settings as you mentioned and load them into quickfixj but it's a significant piece of work and didn't have time to do it (that's why I added configString). There is also the refresh context event (if using Spring Config server) which would require further changes.

I'll try to find the time over holidays to give it a go. Alternatively, feel free to send a PR. All help is welcome.

Thanks

ggershaw commented 1 year ago

Hi,

Happy Holidays. I have not built starter before so it would be nice experience. I will create a branch of main.

What you've created is nothing short of amazing! I just want to put the cherry on top to fully bootify quickFIX.

Best, Geoffrey

Get Outlook for Androidhttps://aka.ms/AAb9ysg


From: Eduardo Sanchez-Ros @.> Sent: Sunday, December 25, 2022 5:01:02 AM To: esanchezros/quickfixj-spring-boot-starter @.> Cc: Raleigh Fix Fan @.>; Mention @.> Subject: Re: [esanchezros/quickfixj-spring-boot-starter] Create well-known properties under quickfixj.server.config (Issue #96)

Hi @ggershawhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fggershaw&data=05%7C01%7C%7C236680f07bc44a405cbe08dae65eef75%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C638075592664316416%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=XVt4doSJrVE1CryUjy79ADWZg1KyMHLbmhWRdZTE%2Baw%3D&reserved=0, thanks for your suggestions. I think they are valid and probably the next logical step in improving the starter.

I myself had this same problem, when loading the settings file was only possible via classpath or absolute path. I thought about creating a replica of all quickfixj config settings as you mentioned and load them into quickfixj but it's a significant piece of work and didn't have time to do it (that's why I added configString). There is also the refresh context event (if using Spring Config server) which would require further changes.

I'll try to find the time over holidays to give it a go. Alternatively, feel free to send a PR. All help is welcome.

Thanks

— Reply to this email directly, view it on GitHubhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fesanchezros%2Fquickfixj-spring-boot-starter%2Fissues%2F96%23issuecomment-1364655463&data=05%7C01%7C%7C236680f07bc44a405cbe08dae65eef75%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C638075592664316416%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=ANGghRDpPSvCVWAJgNVsEcnfpqK0HmSUvti3kJuwpRk%3D&reserved=0, or unsubscribehttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAGD4VG2PH2LAUBU7G6HTPG3WPALN5ANCNFSM6AAAAAATIO6F7Y&data=05%7C01%7C%7C236680f07bc44a405cbe08dae65eef75%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C638075592664316416%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=DRWhCouNaEOTTdmuxsZQJcVnBKPEemA%2Bj%2Bb%2FR509CWs%3D&reserved=0. You are receiving this because you were mentioned.Message ID: @.***>

esanchezros commented 1 year ago

Hi @ggershaw

Have you made a start on this? I may have some capacity soon to get on with this feature if you haven't started yet.

Thanks

ggershaw commented 1 year ago

Hey @esanchezros ,

I think we should test the performance capability of this solution before you put more effort into it. Let me re-read about spring eventing. I think their might be aync event, but still not sure it's built to handle market data.

If I can't assess from the documentation, the branch I posted on the qfix group will send a bunch of MDIR messages from the server in response to an MDR, which are running in separate processes. I could add code to the FromApp call In the client side to look for delays in the messages.

esanchezros commented 1 year ago

Hi @ggershaw,

As I said on that post, Spring Eventing is there as a default implementation and should not be used for high volumes like Market Data. Also, using Async will not guarantee message ordering. There is no issue with performance as far as I know, remember this starter is just a wrapper around quickfixj.

Why don't you provide your own quickfix.Applicationimplemtation? The spring boot starter allows you to override it just by providing a bean of that type. We have services providing market data at a very high throughput.

In this example you can see how to override the default implementation of quickfix.Application. https://github.com/esanchezros/quickfixj-spring-boot-starter-examples/blob/master/simple-server/src/main/java/io/allune/quickfixj/spring/boot/starter/examples/server/ServerApplicationAdapter.java.

ggershaw commented 1 year ago

Got it. Will adjust