Implement the builder pattern for generated datatypes

[andredasilvapinto]

Currently maven-idltods-plugin generates the code by following the Java beans pattern. A builder pattern would enable the created objects to be immutable and provide a more elegant solution for their construction.

[eswdd]

Changing everything to builder pattern would be great for new code, but would require a massive change for all the serialisation/deserialisation code plus users' existing code. We'd planned to use the builder pattern alongside the existing Java beans style.

[andredasilvapinto]

Thanks for the quick reply. Seems fair to me.

[andredasilvapinto]

Just faced a situation where this pattern would have avoided a race condition in production and felt like sharing it here in order to provide a real world case to attest the importance of this change.

One of our services was using the same generated TO to call the same Cougar service (via the respective Cougar client) twice. The second call was being made with a modified TO, and even though Cougar creates new argument arrays on each operation call, e.g.:

new Object[] { textQuery , filter , facets , currencyCode , locale }

the content itself is not copied, so it can be affected by external changes. In our case, the changes performed to the TOs were (most of the time) being applied even before the request was sent to the service. Thus, those two slightly different calls were actually producing two equal HTTP requests to the service, both with the 2nd call arguments.

Even though the bug was there since several months ago, for some reason the race condition started to happen with much more frequency after upgrading to Cougar 3... Maybe it takes longer from calling the Cougar client to actually send the request in Cougar 3 compared with Cougar 2?

Anyway, all this would situation would be avoided with immutable generated TOs. I'm looking forward for this.

[eswdd]

Just realised that the current impl doesn't seal TOs so they remain mutable. Have reopened.

[eswdd]

Surprised that Cougar 3 is much slower than Cougar 2, I don't think much has changed in the client - barring some QoS features. I wonder if someone with access to Cougar 2 might be able to verify any performance changes?

How immutable were you hoping the generated TOs to be? All the way down the tree?

Strikes me that the code you mention is unsafe in a multi-threaded environment and that whilst a builder of immutable objects would prevent you have to wonder at the sanity of the person who wrote it ;)

[andredasilvapinto]

This happened with the 2 machines out of 19 that had cougar 3 but those machines also had Java 8 vs the other ones with Java 7 and cougar 2. So something there has changed which made the race condition more noticeable. Cougar 3 slowness to trigger the request was just a possibility.

I think unless there are really good/proven reasons to use mutable objects, immutability should always be preferred for multi-threaded Apis. Guava also has support for some immutable collections if you want to go that way. Otherwise at least TO immutability as in no setters would be better than what we have right now.

The bug is definitely mainly on the application code (in this case it was caused by me while trying to optimise memory usage by reutilising objects, the to has lots of properties and only one of them changes between requests), but imo cougar (or any other platform) should not assume a specific usage without trying to enforce the contract whenever it is possible to do so in the code. Mistakes will always occur, if we can avoid some of them it is already great.

No dia 17/11/2014, às 22:14, Simon Matic Langford notifications@github.com escreveu:

Surprised that Cougar 3 is much slower than Cougar 2, I don't think much has changed in the client - barring some QoS features. I wonder if someone with access to Cougar 2 might be able to verify any performance changes?

How immutable were you hoping the generated TOs to be? Bearing in mind they use collections etc and I could see benefit in generating mutable objects too I suspect I might close this issue and open a new one for the immutable bits.

Strikes me that the code you mention is unsafe in a multi-threaded environment and that whilst a builder of immutable objects would prevent you have to wonder at the sanity of the person who wrote it ;)

—
Reply to this email directly or view it on GitHub.

[eswdd]

We certainly know Cougar integration tests fail on Java 8 so don't
currently recommend it.

As mentioned previously we can't change the contract ofvthe generated code
without breaking many apps and significant turmoil in the code base.

I was thinking of the build() method taking a boolean as to whether to
generate immutable objects, or adding a seal() method to the builders, or
perhaps defaulting to immutable and having a method to preserve mutability
after calling build().

I quite like the latter in fact - builder created TOs are immutable by
default but existing code gets mutable ones.
On 17 Nov 2014 22:39, "André Pinto" notifications@github.com wrote:

This happened with the 2 machines out of 19 that had cougar 3 but those
machines also had Java 8 vs the other ones with Java 7 and cougar 2. So
something there has changed which made the race condition more noticeable.
Cougar 3 slowness to trigger the request was just a possibility.

I think unless there are really good/proven reasons to use mutable
objects, immutability should always be preferred for multi-threaded Apis.
Guava also has support for some immutable collections if you want to go
that way. Otherwise at least TO immutability as in no setters would be
better than what we have right now.

The bug is definitely mainly on the application code (in this case it was
caused by me while trying to optimise memory usage by reutilising objects,
the to has lots of properties and only one of them changes between
requests), but imo cougar (or any other platform) should not assume a
specific usage without trying to enforce the contract whenever it is
possible to do so in the code. Mistakes will always occur, if we can avoid
some of them it is already great.

No dia 17/11/2014, às 22:14, Simon Matic Langford <
notifications@github.com> escreveu:

Surprised that Cougar 3 is much slower than Cougar 2, I don't think much
has changed in the client - barring some QoS features. I wonder if someone
with access to Cougar 2 might be able to verify any performance changes?

How immutable were you hoping the generated TOs to be? Bearing in mind
they use collections etc and I could see benefit in generating mutable
objects too I suspect I might close this issue and open a new one for the
immutable bits.

Strikes me that the code you mention is unsafe in a multi-threaded
environment and that whilst a builder of immutable objects would prevent
you have to wonder at the sanity of the person who wrote it ;)

—
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHub
#2 (comment).

[andredasilvapinto]

Seems right to me.

Those integration test errors you are talking about are these right?
https://s3.amazonaws.com/archive.travis-ci.org/jobs/23741796/log.txt

We have 1 service with Cougar 3 and Java 8 on the dev stages for almost a month now and we had no problems yet. It is in production on 2/19 boxes without any problem besides the one reported above. The only difference to the main 3.1.0 we added was an override to fix #84 till 3.2.0 is released. Talking about that it seems strange that such a bug didn't get caught by those integration tests.

Tomorrow I'll try to take a look at the Travis errors.

[eswdd]

Its likely/possible our integration tests are incomplete in this regard in
whuch case we should bolster them.
On 17 Nov 2014 23:06, "André Pinto" notifications@github.com wrote:

Seems right to me.

Those integration test errors you are talking about are these right?
https://s3.amazonaws.com/archive.travis-ci.org/jobs/23741796/log.txt

We have 1 service with Cougar 3 and Java 8 on the dev stages for almost a
month now and we had no problems yet. It is in production on 2/19 boxes
without any problem besides the one reported above. The only difference to
the main 3.1.0 we added was an override to fix #84
#84 till 3.2.0 is released.
Talking about that it seems strange that such a bug didn't get caught by
those integration tests.

Tomorrow I'll try to take a look at the Travis errors.

—
Reply to this email directly or view it on GitHub
#2 (comment).

[andredasilvapinto]

So it took me a bit longer than I expected, but I managed to get the integration tests passing with Java 8.

All the 158 errors reported on #69 were not being caused by any incompatibility with Java 8 but by badly designed assertions. Basically the tests are depending on the order of several elements in the response, which should be irrelevant. In some cases I have changed code in order to fix that behaviour, in other cases I have just changed the order of the elements in the expected object to fit the response.

Here are the problems that I have identified:

• Some JSON replies are being compared as strings
• Order matters in JSON equality assertions

expected:<{"id":1,"error":{"message":"DSC-0015","code":-32099},"jsonrpc":"2.0"}> but was:<{"id":1,"jsonrpc":"2.0","error":{"code":-32099,"message":"DSC-0015"}}>

• Binary Protocol tests do not ensure there are active sessions to the server before running
• Set order in response order matters

expected:<[aaa, ddd, ccc, bbb]> but was:<[aaa, ccc, bbb, ddd]>

• Order matters for document assertions

java.lang.AssertionError: Expected: aaa stringccc stringbbb stringddd string, actual: aaa stringbbb stringccc stringddd string: Node node value check: Expected :bbb string Actual :ccc string

• Order of array elements matters for id extraction on com.betfair.testing.utils.cougar.helpers.CougarHelpers#convertBatchedResponseToMap
• com.betfair.testing.utils.cougar.assertions.AssertionUtils#jettAssertEquals doesn't print the expected vs actual message when comparing with null

I also don't understand the need for manual JSON and XML equality implementations when libraries like Jackson and XMLUnit can give us that for free.

I have pushed a version with which you should be able to get a successful test run with Java 8, andredasilvapinto@d05c84b . Feel free to use it as you find useful.

In any case, it seems that there is no apparent reason to avoid using Java 8 with Cougar 3, or do you know any other issue with it?

[eswdd]

Cool! Could you possibly move this comment to #69 and submit a pull request so travis can verify your changes on Java 7? I'll answer your questions/comments there..