Diffa Diffa → 0.9.3

Comments and changes to this ticket

• 

  Ben Hood October 29th, 2010 @ 11:47 AM

  • Assigned user set to “Tim Clark”
• 

  Ben Hood December 23rd, 2010 @ 02:00 PM

  • Assigned user changed from “Tim Clark” to “John Wright”
• 

  Ben Hood January 17th, 2011 @ 12:02 PM

  • State changed from “new” to “open”

  [bulk edit]

• You flagged this item as spam.
  

  John Wright January 19th, 2011 @ 11:37 AM

  • State changed from “open” to “resolved”

  Fixed in branch 143, fork at git://github.com/johnwright/diffa.git

• 

  Ben Hood January 19th, 2011 @ 12:37 PM

  • State changed from “resolved” to “open”

  In general, it looks like a good change, but there are few things that would need to get brushed up.

  QA notes:

  • If Rabbit is not running locally, then the AMQP based tests fail, causing the build to fail:

    Tests in error: 
    pingPong(net.lshift.diffa.messaging.amqp.AmqpRpcTests)
    receiveErrorCode(net.lshift.diffa.messaging.amqp.AmqpRpcTests)
    emptyResponse(net.lshift.diffa.messaging.amqp.AmqpRpcTests)
    

  • AMQPQueueURl:

    • It seems like something you might want a small test for

  • AMQRpcClient:

    • Is L29 necessary?

      m.setQueueName("temp")// will be replaced when the channel is set up
      

  • ChangesAmqpClient:

    • Where is this class being used?

  • JsonAmqpMessagingRegistrar:

    • What is the purpose of the commented out code?

  • JsonAmqpParticipantProtocolFactory:

    • It might be a good idea to put the RPC timeout into a properties file

  • AbstractJSONHandler:

    • I'm not getting how the 1-way RPC is being replaced in the new version, but maybe I'm missing something here - perhaps you could explain it to me.

  • AMQP pom.xml:

    • Is the Spring AMQP version being used?
    • Why are repositories commented out?

  • jetty-env.xml:

    • There is some commented out code here

  • JSONEncodingUtils:

    • serializeEmptyResponse - this potentially ties in with the 1-way RPC comment from above - I'd just like clarification of this

  • transports.tex:

    • I like the effort you went to in spite of the poor precedent that was set, so this is good.

  • Stylistically:

    • A lot of (new) classes do not have sufficient documentation on them - if you are in doubt about the precedent that has been set, then just have a quick chat with me to clarify what the expectation is
    • Most new classes do not have license headers
    • The Maven module should not be unnamed:

      ------------------------------------------------------------------------
      [INFO] Reactor Summary:
      [INFO] ------------------------------------------------------------------------
      [INFO] JavaMail File Transport ............................... SUCCESS [0.471s]
      [INFO] LShift Diffa Parent POM ............................... SUCCESS [0.043s]
      [INFO] LShift Diffa Kernel ................................... SUCCESS [0.151s]
      [INFO] LShift Diffa JSON Messaging Support ................... SUCCESS [0.004s]
      [INFO] LShift Diffa Participant Support ...................... SUCCESS [0.040s]
      [INFO] Unnamed - net.lshift.diffa:diffa-amqp-messaging:jar:0.9.1-SNAPSHOT  SUCCESS [0.039s]
      [INFO] LShift Diffa Tools .................................... SUCCESS [0.004s]
      [INFO] LShift Diffa docgen-maven-plugin Maven Mojo ........... SUCCESS [0.004s]
      [INFO] LShift Diffa Agent .................................... SUCCESS [0.136s]
      [INFO] LShift Diffa Web Participants ......................... SUCCESS [0.059s]
      [INFO] LShift Diffa Dist Packaging ........................... SUCCESS [0.059s]
      [INFO] ------------------------------------------------------------------------
      

• 

  Ben Hood January 19th, 2011 @ 01:01 PM

  Also, don't forget to merge master back into your branch since #148 has landed in the meantime.

• You flagged this item as spam.
  

  John Wright January 20th, 2011 @ 03:29 PM

  • State changed from “open” to “resolved”
  • Assigned user changed from “John Wright” to “Ben Hood”

  Updated for QA notes, merged changes for #148 from master

• You flagged this item as spam.
  

  John Wright January 20th, 2011 @ 06:18 PM

  • State changed from “resolved” to “open”
  • Assigned user changed from “Ben Hood” to “John Wright”

  Found some failing tests, reopening.

• You flagged this item as spam.
  

  John Wright January 21st, 2011 @ 10:12 AM

  The following agent integration tests failed yesterday evening:

  walkThroughEventDetails(net.lshift.diffa.agent.itest.CorrelatedEnvironmentTest)
  shouldNotFindDifferencesInParticipantsWithSameStateThatAgentWasntInformedOf(net.lshift.diffa.agent.itest.CorrelatedEnvironmentTest)

  Mysteriously, they're working again this morning and I haven't been able to reproduce the failure after four runs.

• You flagged this item as spam.
  

  John Wright January 21st, 2011 @ 11:40 AM

  • State changed from “open” to “resolved”
  • Assigned user changed from “John Wright” to “Ben Hood”
• 

  Ben Hood January 21st, 2011 @ 02:43 PM

  (from [24c9a7e3b1f360ae5a7789b1841c98cf3b27e22b]) Merged [#143] into master https://github.com/lshift/diffa/commit/24c9a7e3b1f360ae5a7789b1841c...

• 

  Ben Hood January 21st, 2011 @ 02:49 PM

  • State changed from “resolved” to “closed”

  QA+

• 

  Ben Hood February 4th, 2011 @ 02:48 PM

  • Milestone set to “0.9.3”
  • Milestone order changed from “10” to “0”

  [bulk edit]