[GitHub] [geode] balesh2 opened pull request #3386: GEODE-6582: get max message size only once

["balesh2 (GitHub)" <gi...@apache.org>]

The max message size did not change over the course of execution but was
being re-fetched every time a new message was constructed. To improve
message performance, this value is now static and fetched only once.

Thank you for submitting a contribution to Apache Geode.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?

- [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?

- [ ] Is your initial contribution a single, squashed commit?

- [ ] Does `gradlew build` run cleanly?

- [ ] Have you written or updated unit tests to verify your changes?

- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?

### Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.


[ Full content available at: https://github.com/apache/geode/pull/3386 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] balesh2 commented on pull request #3386: GEODE-6582: get max message size only once

["balesh2 (GitHub)" <gi...@apache.org>]

👍 

[ Full content available at: https://github.com/apache/geode/pull/3386 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] kirklund commented on pull request #3386: GEODE-6582: get max message size only once

["kirklund (GitHub)" <gi...@apache.org>]

The downside of making it `static final` is that it becomes untestable. No test can specify or change the value. The alternative would be to move it up a level and just pass `int maxMessageSize` in via the constructor. That keeps this class more testable.

[ Full content available at: https://github.com/apache/geode/pull/3386 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] balesh2 commented on pull request #3386: GEODE-6582: get max message size only once

["balesh2 (GitHub)" <gi...@apache.org>]

The maxMessageSize is currently not being changed in any tests, so I think that keeping it this way makes sense. If we set it in the constructor, we would have the same performance issue as we do currently since the value is being retrieved from a system property, which requires locking. I believe that in order to remove that performance issue, the best solution is the make the value static and initialized at the class level. However since it is a common pattern of ours to make values like that non-static for the purposes of testing, I will add a comment explaining why it is static.

[ Full content available at: https://github.com/apache/geode/pull/3386 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] pivotal-jbarrett commented on pull request #3386: GEODE-6582: get max message size only once

["pivotal-jbarrett (GitHub)" <gi...@apache.org>]

Let's remove the reference to the ticket and include any important information from that ticket here in the comment. I doubt there is anything applicable to include though. 

[ Full content available at: https://github.com/apache/geode/pull/3386 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] balesh2 closed pull request #3386: GEODE-6582: get max message size only once

["balesh2 (GitHub)" <gi...@apache.org>]

[ pull request closed by balesh2 ]

[ Full content available at: https://github.com/apache/geode/pull/3386 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] pivotal-jbarrett commented on pull request #3386: GEODE-6582: get max message size only once

["pivotal-jbarrett (GitHub)" <gi...@apache.org>]

I believe there is an annotation for marking it mutable for testing only. 

I agree with both of you. Something is broken here. I am not sure the best way to fix it. Ideally we’d stop doing this stupid system property crap but we just keep adding to that mess. The configuration for this object should come from its parent but that’s the same problem, should it be static there too? Finding a place that is not static but isn’t frequently instantiated so that it can be fetch infrequently would be the best compromise. 

I think I’d like to see this ticket focus on the severe performance problem. And write up another to refactor the relationship such that this doesn’t need to be static but doesn’t get fetch from system properties more than once.

[ Full content available at: https://github.com/apache/geode/pull/3386 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org