You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by "jinmeiliao (GitHub)" <gi...@apache.org> on 2019/01/17 17:40:27 UTC
[GitHub] [geode] jinmeiliao opened pull request #3088: GEODE-6283: have the management rest controller call the internal clu…
…ster management service to actually create the region
* inject cluster management service to the rest controller
* have the controller produce json string for response
* refactor the controller exception handler to always send back json string
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/3088 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jinmeiliao closed pull request #3088: GEODE-6283: have the management rest controller call the internal clu…
Posted by "jinmeiliao (GitHub)" <gi...@apache.org>.
[ pull request closed by jinmeiliao ]
[ Full content available at: https://github.com/apache/geode/pull/3088 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] pdxrunner commented on pull request #3088: GEODE-6283: have the management rest controller call the internal clu…
Posted by "pdxrunner (GitHub)" <gi...@apache.org>.
Typo: `GEode` -> `Geode`
[ Full content available at: https://github.com/apache/geode/pull/3088 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jinmeiliao commented on pull request #3088: GEODE-6283: have the management rest controller call the internal clu…
Posted by "jinmeiliao (GitHub)" <gi...@apache.org>.
we could have a very lean locator that doesn't have management service running, like the locator used by our Dunit launcher. Though it's not recommended, but it's possible.
[ Full content available at: https://github.com/apache/geode/pull/3088 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jinmeiliao commented on pull request #3088: GEODE-6283: have the management rest controller call the internal clu…
Posted by "jinmeiliao (GitHub)" <gi...@apache.org>.
just to avoid having to create a Property and manipulate it based on what webapp it is. one webapp only requires security service, another requires security service and sslConfig, and another requires securityService and managementService. So instead of
```
Properties properties = new Properties();
properties.put("securityService", securityService);
addWebapp(context1, war1, properties);
properties.put("sslConfig", sslConfig);
addWebapp(context2, war2, properties);
```
I could do
```
addWebapp(context1, war1, new Object[]{"securityService", securityService});
addWebapp(context2, war2, new Object[]{"sslConfig", sslConfig}, new Object[]{"securityService", securityService});
```
[ Full content available at: https://github.com/apache/geode/pull/3088 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jdeppe-pivotal commented on pull request #3088: GEODE-6283: have the management rest controller call the internal clu…
Posted by "jdeppe-pivotal (GitHub)" <gi...@apache.org>.
It seems a bit redundant having 'CacheElement' in the name of these methods. It's also not entirely accurate seeing that the methods will result in both persistence and realization. I think they'd be better as just `create`, `update` and `delete`.
[ Full content available at: https://github.com/apache/geode/pull/3088 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] pdxrunner commented on pull request #3088: GEODE-6283: have the management rest controller call the internal clu…
Posted by "pdxrunner (GitHub)" <gi...@apache.org>.
Is this an error or not? Management service not running could lead to unexpected results to the user/operator.
[ Full content available at: https://github.com/apache/geode/pull/3088 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jdeppe-pivotal commented on pull request #3088: GEODE-6283: have the management rest controller call the internal clu…
Posted by "jdeppe-pivotal (GitHub)" <gi...@apache.org>.
It seems a bit redundant having 'CacheElement' in the name of these methods. It's also not entirely accurate seeing that the methods will result in both persistence and realization. I think they'd be better as just `create`, `update` and `delete`
[ Full content available at: https://github.com/apache/geode/pull/3088 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jinmeiliao commented on issue #3088: GEODE-6283: have the management rest controller call the internal clu…
Posted by "jinmeiliao (GitHub)" <gi...@apache.org>.
@aditya87 @Petahhh
[ Full content available at: https://github.com/apache/geode/pull/3088 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jdeppe-pivotal commented on pull request #3088: GEODE-6283: have the management rest controller call the internal clu…
Posted by "jdeppe-pivotal (GitHub)" <gi...@apache.org>.
It seems a bit redundant having 'CacheElement' in the name of these methods. It's also not entirely accurate seeing that the methods will result in both persistence and realization. I think they'd be better as just `cerate`, `update` and `delete`
[ Full content available at: https://github.com/apache/geode/pull/3088 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jdeppe-pivotal commented on pull request #3088: GEODE-6283: have the management rest controller call the internal clu…
Posted by "jdeppe-pivotal (GitHub)" <gi...@apache.org>.
Looking at the code that calls the `addWebapp` method it's fairly obvious what's happening, however the actual method signature of `addWebapp` completely loses any semantic notion that it's being called with tuples of key/value pairs. Sorry if it seems pedantic, but please would you change it. :)
[ Full content available at: https://github.com/apache/geode/pull/3088 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jinmeiliao commented on pull request #3088: GEODE-6283: have the management rest controller call the internal clu…
Posted by "jinmeiliao (GitHub)" <gi...@apache.org>.
sounds good.
[ Full content available at: https://github.com/apache/geode/pull/3088 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jdeppe-pivotal commented on pull request #3088: GEODE-6283: have the management rest controller call the internal clu…
Posted by "jdeppe-pivotal (GitHub)" <gi...@apache.org>.
Seems more obvious to just use a `Properties` object here. I don't know why we made the underlying methods use an array of (implicit) `Object[2]` arguments.
[ Full content available at: https://github.com/apache/geode/pull/3088 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] pdxrunner commented on pull request #3088: GEODE-6283: have the management rest controller call the internal clu…
Posted by "pdxrunner (GitHub)" <gi...@apache.org>.
This log message doesn't seem necessary. If you want to keep it, then the message needs to be more meaningful.
[ Full content available at: https://github.com/apache/geode/pull/3088 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jdeppe-pivotal commented on pull request #3088: GEODE-6283: have the management rest controller call the internal clu…
Posted by "jdeppe-pivotal (GitHub)" <gi...@apache.org>.
It seems a bit redundant having 'CacheElement' in the name of these methods. It's also not entirely accurate seeing that the methods will result in both persistence and realization.
[ Full content available at: https://github.com/apache/geode/pull/3088 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org