[GitHub] [geode] alb3rtobr opened pull request #4067: GEODE-6871: Disk free space info exposed via JMX

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

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:
- [x] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?

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

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

- [x] Does `gradlew build` run cleanly?

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

- [n/a] 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/4067 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] mhansonp closed pull request #4067: GEODE-6871: Disk free space info exposed via JMX

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

[ pull request closed by mhansonp ]

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

[GitHub] [geode] jujoramos commented on pull request #4067: GEODE-6871: Disk free space info exposed via JMX

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

Since you're already modifying the test name, can you please remove the underscores and use camel case instead?.

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

[GitHub] [geode] dschneider-pivotal commented on pull request #4067: GEODE-6871: Disk free space info exposed via JMX

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

@jujoramos I like the idea of adding a default to interface methods we add to existing external interfaces.

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

[GitHub] [geode] alb3rtobr commented on pull request #4067: GEODE-6871: Disk free space info exposed via JMX

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

As this test uses given-when-then style, I find its easy to understand what the test is doing by separating the statements, but I understand its not the standard in all the tests, so I can change it.

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

[GitHub] [geode] dschneider-pivotal commented on pull request #4067: GEODE-6871: Disk free space info exposed via JMX

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

Geode exposes much of the external apis on interfaces. Many of those interfaces are not meant to be implemented by geode users. Instead they are used to hide the internal geode implementation. For example Region is an interface that a geode user never needs to implement. So in the past as we added new features to geode adding a method to one of these interfaces was considered not breaking backward compatibility. But in the past we didn't even have the option of "default" methods on interfaces. The external geode api also has interfaces that only exist for geode users to implement. An example is CacheListener. It seems to me like a good rule going forward that if we add a method on an existing external interface that we use "default" on it to be safe. It can be unclear if an interface is meant to be implemented, and it is always on option and a geode user might implement Region for testing purposes or to have their own wrapper around the actual geode Region.

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

[GitHub] [geode] jujoramos commented on pull request #4067: GEODE-6871: Disk free space info exposed via JMX

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

I'm not sure why we didn't consider "worth it" adding a `default` implementation for `getDiskUsagePercentage` in [GEODE-5222](https://issues.apache.org/jira/browse/GEODE-5222), I'm tagging the original reviewers as they might have more context than me on this one.
Either way, unless there's a planned major version release in the near future, I just don't like the idea of breaking the backward compatibility in our public classes... even if they are rarely implemented by our users, we can't tell for sure.

@moleske @kirklund @aaronlindsey 

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

[GitHub] [geode] jujoramos commented on pull request #4067: GEODE-6871: Disk free space info exposed via JMX

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

@dschneider-pivotal: I'm not sure about what happens when code compiled against the old interface is executed using the new one, haven't tried yet.
That said, user code implementing the old version of the interface will certainly not compile if we don't provide a `default` implementation, that already sounds like an API change that is no backward compatible to me (please correct me if I'm wrong here).

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

[GitHub] [geode] aaronlindsey commented on pull request #4067: GEODE-6871: Disk free space info exposed via JMX

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

> Should I remove them or keep them?

As a general rule, I'd say adhere to the existing style. However, in Geode we don't seem to have a standard style for test names. I personally prefer the style with the underscores, but I'd be fine with it either way.



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

[GitHub] [geode] alb3rtobr commented on issue #4067: GEODE-6871: Disk free space info exposed via JMX

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

> Seems alright to me, I appreciate the descriptive test names!

Its better to use descriptive names than comments explaining the test case :) Thanks!

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

[GitHub] [geode] dschneider-pivotal commented on pull request #4067: GEODE-6871: Disk free space info exposed via JMX

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

@jujoramos Does the addition of a new method on an interface break old code that is already compiled against the old interface? When you say it "breaks backward compatibility" what do you mean? Is it that the old class files will no longer run with the new product jar? Or is it that old code will no longer compile? In the past we treated method additions as compatible, but removing methods and changing existing methods as incompatible.

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

[GitHub] [geode] alb3rtobr commented on pull request #4067: GEODE-6871: Disk free space info exposed via JMX

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

I have added default implementation for both methods. I suppose the default implementation of `getDiskUsagePercentage` should be included in 1.10 before release, right?

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

[GitHub] [geode] alb3rtobr commented on pull request #4067: GEODE-6871: Disk free space info exposed via JMX

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

This PR is based on GEODE-5222, and in that PR it was considered ok to modify the interface to add "getDiskUsagePercentage" without default implementation, I suppose it was due to its not usually implemented as you said.


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

[GitHub] [geode] aaronlindsey commented on pull request #4067: GEODE-6871: Disk free space info exposed via JMX

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

> I'm not sure why we didn't consider "worth it" adding a `default` implementation for `getDiskUsagePercentage` in [GEODE-5222](https://issues.apache.org/jira/browse/GEODE-5222), I'm tagging the original reviewers as they might have more context than me on this one.

I think it's a good idea to add a default implementation for both methods. At the time I was very new to this project so I didn't consider the possibility of a breaking change. I'm not sure whether this would be considered a "critical" fix to make it in 1.10. I suppose this might be a good question for the dev list.



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

[GitHub] [geode] jujoramos commented on pull request #4067: GEODE-6871: Disk free space info exposed via JMX

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

This is a public interface, even though I rarely see people manually implementing it, the addition of a new method would break backward compatibility... would it make sense to add a `default` implementation?.

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

[GitHub] [geode] dschneider-pivotal commented on pull request #4067: GEODE-6871: Disk free space info exposed via JMX

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

I think this mix of camel case and underscores was intentional to help highlight the three sections (given, when, then). @dhemery may have some helpful input on this.

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

[GitHub] [geode] aaronlindsey commented on pull request #4067: GEODE-6871: Disk free space info exposed via JMX

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

I think the underscores help make the test name more readable in this case (because of the given/when/then style and the long length of the name).

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

[GitHub] [geode] jujoramos commented on pull request #4067: GEODE-6871: Disk free space info exposed via JMX

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

The name should read something like `givenDiskStoreIsCreatedWithMaxSizeThenDiskUsagePercentageShouldBeZeroAndDiskFreePercentageShouldBe100`.

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