[GitHub] [geode] jdeppe-pivotal commented on pull request #3088: GEODE-6283: have the management rest controller call the internal clu…
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] jdeppe-pivotal commented on pull request #3088: GEODE-6283: have the management rest controller call the internal clu…
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] jdeppe-pivotal commented on pull request #3088: GEODE-6283: have the management rest controller call the internal clu…
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] jdeppe-pivotal commented on pull request #3088: GEODE-6283: have the management rest controller call the internal clu…
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…
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
[GitHub] [geode] jdeppe-pivotal commented on pull request #3088: GEODE-6283: have the management rest controller call the internal clu…
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
