Uploaded image for project: 'QuickFIX/J'
  1. QuickFIX/J
  2. QFJ-802

mvn message module : cut circular dependancy w/ core + merge sub-modules

    Details

    • Type: Improvement
    • Status: Open
    • Priority: Minor
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: Build
    • Labels:
      None

      Description

      Core module currently depends on resources located in the messages modules (hidden dependancy), while messages module depend on core module (explicit dependancy).
      This prevent from importing in Eclipse IDE without manual operations.

      The object of the ticket is to check if :

      • Dependancy can be declared such as core depends on messages, while cutting dependancy from messages to core
      • Make one single message mvn module

        Attachments

          Activity

          Hide
          chrjohn Christoph John added a comment -

          Hi, we already have QFJ-782 but it does not have very much description.
          Do you see any advantage in having one single message module?

          Show
          chrjohn Christoph John added a comment - Hi, we already have QFJ-782 but it does not have very much description. Do you see any advantage in having one single message module?
          Hide
          manureno ManuReno added a comment -

          I thought about 1 single message module since :

          • the quickfix.Message class rely on generated code not present in all FIX dictionaries
            quickfix.Message class uses SessionRejectReason which is not available in FIX40 nor FIX41
            throw new FieldException(SessionRejectReason.TAG_APPEARS_MORE_THAN_ONCE, field.getTag());
            
          • While the FIX-generated classes (even FIX40 and FIX41) all depend back on this Message class.

          This looks like using the jar quickfixj-msg-fix40.jar would require quickfixj-core.jar (for the Message class) and quickfixj-msg-fix42.jar or later (for the SessionRejectReason class) on its classpath.
          Considering that all jars seems required everytime, why not one single jar with all messages in it ?...

          When trying this I faced a compilation issue : it all the FIX-generated classes couldn't be compiled together in my environment (OOM error during compilation whatever the max heapsize, either on Java6 or Java7 JDK ) when FIX50-SPxx are present.
          BTW one can see that all FIX dictionaries but FIX50-SPxx are generated and compiled in the core module so I guess that someone else has already faced the issue...

          Maybe the message modules should be kept, but while ensuring that the classes used by the generated code are not using back this generated code (namely DataDictionary, DataDictionaryProvider, FieldMap, Message, MessageUtils, SessionID, SessionRejectReasonText classes).

          Show
          manureno ManuReno added a comment - I thought about 1 single message module since : the quickfix.Message class rely on generated code not present in all FIX dictionaries quickfix.Message class uses SessionRejectReason which is not available in FIX40 nor FIX41 throw new FieldException(SessionRejectReason.TAG_APPEARS_MORE_THAN_ONCE, field.getTag()); While the FIX-generated classes (even FIX40 and FIX41) all depend back on this Message class. This looks like using the jar quickfixj-msg-fix40.jar would require quickfixj-core.jar (for the Message class) and quickfixj-msg-fix42.jar or later (for the SessionRejectReason class) on its classpath. Considering that all jars seems required everytime, why not one single jar with all messages in it ?... When trying this I faced a compilation issue : it all the FIX-generated classes couldn't be compiled together in my environment (OOM error during compilation whatever the max heapsize, either on Java6 or Java7 JDK ) when FIX50-SPxx are present. BTW one can see that all FIX dictionaries but FIX50-SPxx are generated and compiled in the core module so I guess that someone else has already faced the issue... Maybe the message modules should be kept, but while ensuring that the classes used by the generated code are not using back this generated code (namely DataDictionary, DataDictionaryProvider, FieldMap, Message, MessageUtils, SessionID, SessionRejectReasonText classes).
          Hide
          manureno ManuReno added a comment -

          Difference in the quickfixj-messages-all artifacts before and after the build structure change

          Show
          manureno ManuReno added a comment - Difference in the quickfixj-messages-all artifacts before and after the build structure change
          Hide
          manureno ManuReno added a comment -

          A pull request is available for the change related to this ticket.

          In attachement of this Jira are listed the differences in the quickfixj-messages-all artifacts that result from the build structure change.

          It is not possible to use anymore standalone quickfixj-messages-[fix40|fix41|fix42|fix43|fix44].jar files.
          But due to the circular dependancies described above, I guess this was a bit risky anyway (if somehow possible).
          If you think this makes an issue, let me know.

          The mvn artifact structure is now explicitely described in the POMs and the build is also faster.

          Tests pass on my machine except 2 of them for which it looks there is a race condition (sometimes 4 are failing, sometime 2 or 3...) even after a clean checkout
          The failures are attached, BTW I'm interested by a tip to make them pass.

          Show
          manureno ManuReno added a comment - A pull request is available for the change related to this ticket. In attachement of this Jira are listed the differences in the quickfixj-messages-all artifacts that result from the build structure change. It is not possible to use anymore standalone quickfixj-messages- [fix40|fix41|fix42|fix43|fix44] .jar files. But due to the circular dependancies described above, I guess this was a bit risky anyway (if somehow possible). If you think this makes an issue, let me know. The mvn artifact structure is now explicitely described in the POMs and the build is also faster. Tests pass on my machine except 2 of them for which it looks there is a race condition (sometimes 4 are failing, sometime 2 or 3...) even after a clean checkout The failures are attached, BTW I'm interested by a tip to make them pass.
          Hide
          chrjohn Christoph John added a comment - - edited

          Sorry, did not find time to review the pull request yet.
          I have some questions:

          • You say it is not possible to use the fix4* standalone files. What about the fix5* files?
          • Are always the 19b_PossResendMessageThatHasNotBeenSent.def tests failing or also others?
            How do the exchanged messages for these tests look like? When the tests expects a NewOrderSingle but gets only a Heartbeat it seems either like the Session did not recognize the NewOrderSingle or it has not been sent and the Session is simply heartbeating.
          Show
          chrjohn Christoph John added a comment - - edited Sorry, did not find time to review the pull request yet. I have some questions: You say it is not possible to use the fix4* standalone files. What about the fix5* files? Are always the 19b_PossResendMessageThatHasNotBeenSent.def tests failing or also others? How do the exchanged messages for these tests look like? When the tests expects a NewOrderSingle but gets only a Heartbeat it seems either like the Session did not recognize the NewOrderSingle or it has not been sent and the Session is simply heartbeating.
          Hide
          manureno ManuReno added a comment -

          Hi Christoph

          After reviewing all, the potential issue (conflicting field classes) can be worked around if the core module is first on the classpath.
          There may be other options with less impact on the build to deal with that : please don't review the pull request yet, I'll think about something else and let you know.

          Show
          manureno ManuReno added a comment - Hi Christoph After reviewing all, the potential issue (conflicting field classes) can be worked around if the core module is first on the classpath. There may be other options with less impact on the build to deal with that : please don't review the pull request yet, I'll think about something else and let you know.

            People

            • Assignee:
              manureno ManuReno
              Reporter:
              manureno ManuReno
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated: