You are viewing a plain text version of this content. The canonical link for it is here.

Posted to notifications@geode.apache.org by "jdeppe-pivotal (GitHub)" <gi...@apache.org> on 2019/02/13 22:31:18 UTC

[GitHub] [geode] jdeppe-pivotal opened pull request #3191: GEODE-6384: Create consistent API to retrieve instances of ClusterManagementService

- The overall intent of this commit is to provide a consistent means for
retrieving a ClusterMangementService instance regardless of the
context you are developing in. Currently, the relevant contexts are:
1) a Java application that has no access (or need to access) core
Geode code. 2) a typical Geode client application and 3) server or
locator-side code.
- Introduces a service provider interface
(ClusterManagementServiceProviderFactory) whose implementations are
context dependent. This commit introduces two implementations, namely:
BasicClusterManagementProviderFactory and
SmartClusterManagementServiceProviderFactory). The latter is
incomplete but will be fully supported in the near future.
- The entry point to creating a ClusterManagementService is in
ClusterManagementServiceProvider - more documentation can be found
there.

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, you check travis-ci 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/3191 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] jinmeiliao commented on pull request #3191: GEODE-6384: Create consistent API to retrieve instances of ClusterManagementService

Posted by "jinmeiliao (GitHub)" <gi...@apache.org>.

Do we want to change this error handler to something else? can we make`RestTemplateResponseErrorHandler` an inner class to contain the error handling in the client?

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

[GitHub] [geode] jdeppe-pivotal closed pull request #3191: GEODE-6384: Create consistent API to retrieve instances of ClusterManagementService

Posted by "jdeppe-pivotal (GitHub)" <gi...@apache.org>.

[ pull request closed by jdeppe-pivotal ]

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

[GitHub] [geode] pdxrunner commented on pull request #3191: GEODE-6384: Create consistent API to retrieve instances of ClusterManagementService

Posted by "pdxrunner (GitHub)" <gi...@apache.org>.

As noted above, All the new public API classes need to have javadocs and `@Experimental` annotations

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

[GitHub] [geode] jinmeiliao commented on pull request #3191: GEODE-6384: Create consistent API to retrieve instances of ClusterManagementService

Posted by "jinmeiliao (GitHub)" <gi...@apache.org>.

I am not sure if we need all those context....

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

[GitHub] [geode] jinmeiliao commented on pull request #3191: GEODE-6384: Create consistent API to retrieve instances of ClusterManagementService

Posted by "jinmeiliao (GitHub)" <gi...@apache.org>.

The way I understand is, for developer facing, there are two ways to get the CMS, one is without the url (in the server context), one is with the url (in the client context, either with a url or with an HttpRequestFactory). Can we put all the service create logic in this `ClusterManagementServiceProvider` class instead of going through all the factories? i.e, your getService() method body is the `SmartClusterManagementServiceProviderFactory` create, and getService(url) is the "BasicClusterManagementServiceProviderFactory` create. This seems much easier to follow and without all these service loader stuff.

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

[GitHub] [geode] jinmeiliao commented on pull request #3191: GEODE-6384: Create consistent API to retrieve instances of ClusterManagementService

Posted by "jinmeiliao (GitHub)" <gi...@apache.org>.

Isn't this the same as `GeodeClientClusterManagementService` and `ServerClusterManagementService`?

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

[GitHub] [geode] jdeppe-pivotal commented on issue #3191: GEODE-6384: Create consistent API to retrieve instances of ClusterManagementService

Posted by "jdeppe-pivotal (GitHub)" <gi...@apache.org>.

@Petahhh @onichols-pivotal For your review...

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

[GitHub] [geode] jinmeiliao commented on pull request #3191: GEODE-6384: Create consistent API to retrieve instances of ClusterManagementService

Posted by "jinmeiliao (GitHub)" <gi...@apache.org>.

is this test needed?

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

[GitHub] [geode] jinmeiliao commented on pull request #3191: GEODE-6384: Create consistent API to retrieve instances of ClusterManagementService

Posted by "jinmeiliao (GitHub)" <gi...@apache.org>.

should this also have LOCATOR_CONTEXT? or if the create() method supports both "LOCATOR" and "SERVER" mode, then we only need one string for this.

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

[GitHub] [geode] pdxrunner commented on pull request #3191: GEODE-6384: Create consistent API to retrieve instances of ClusterManagementService

Posted by "pdxrunner (GitHub)" <gi...@apache.org>.

This needs a javadoc. Also an `@Experimental` annotation

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

[GitHub] [geode] jdeppe-pivotal commented on pull request #3191: GEODE-6384: Create consistent API to retrieve instances of ClusterManagementService

Posted by "jdeppe-pivotal (GitHub)" <gi...@apache.org>.

I've added some javadoc and also moved the class to `internal` as it isn't an API a user would directly interact with.

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

[GitHub] [geode] jinmeiliao commented on pull request #3191: GEODE-6384: Create consistent API to retrieve instances of ClusterManagementService

Posted by "jinmeiliao (GitHub)" <gi...@apache.org>.

this list should only be SERVER_CONTEXT, LOCATOR_CONTEXT and GEODE_CLIENT_CONTEXT, because only those context support/will support create() method without parameter.

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

[GitHub] [geode] jdeppe-pivotal commented on pull request #3191: GEODE-6384: Create consistent API to retrieve instances of ClusterManagementService

Posted by "jdeppe-pivotal (GitHub)" <gi...@apache.org>.

I've deleted this class as it's not necessary at the moment.

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