[GitHub] [geode] dschneider-pivotal commented on pull request #2918: GEODE-6102: add gfsh destroy data-source
Another complication: when you call JNDIInvoker.getDataSource() it when return
null for things that do exist (i.e. were created by create jndi-binding) but
that can not be used as a "DataSource". It will also return null if it does not
exist. In this context I think you need to know that the given name exists in
the "dataSourceMap" but that it is not an instance of DataSource. If that is
true then you can't destroy it but need to tell them to use destroy
jndi-binding. No need to do the instanceof checks. You could add a method on
JNDIInvoker like this:
public boolean existsInDataSourceMap(String name) {
return dataSourceMap.containsKey(name);
}
You could then use this method to figure out if getting null back from
getDataSource means it does not exist at all or if it exists but is not a
"DataSource".
[ Full content available at: https://github.com/apache/geode/pull/2918 ]
This message was relayed via gitbox.apache.org for
notifications@geode.apache.org
[GitHub] [geode] dschneider-pivotal commented on pull request #2918: GEODE-6102: add gfsh destroy data-source
I think this expression could be cleaned up a bit.
use instanceof instead of .getClass() ==. Also with instanceof no need for a
null check.
So you could have a method like this:
private boolean wasCreatedAsDataSource(DataSource dataSource) {
return dataSource instanceof GemFireBasicDataSource
|| dataSource instanceof GemFireConnPoolDataSource;
}
The in this code you could just say:
if (!wasCreatedAsDataSource(dataSource) {
error...
[ Full content available at: https://github.com/apache/geode/pull/2918 ]
This message was relayed via gitbox.apache.org for
notifications@geode.apache.org
[GitHub] [geode] dschneider-pivotal commented on pull request #2918: GEODE-6102: add gfsh destroy data-source
It seems like a mock should be used instead of adding a new class (i.e. InvalidDataSourceClass) to the test. [ Full content available at: https://github.com/apache/geode/pull/2918 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] dschneider-pivotal commented on pull request #2918: GEODE-6102: add gfsh destroy data-source
I think it just allows you to run all gfsh tests from gradle. We also have a category for jdbcTests and a chore that we should add it to all the jdbc commands. [ Full content available at: https://github.com/apache/geode/pull/2918 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] dschneider-pivotal commented on pull request #2918: GEODE-6102: add gfsh destroy data-source
This comment came from DestroyJndiBinding. We will update it. [ Full content available at: https://github.com/apache/geode/pull/2918 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] dschneider-pivotal commented on pull request #2918: GEODE-6102: add gfsh destroy data-source
Because the existing "destroy jndi-binding" did not want to report a problem if a server did not have the jndi-binding. This also looked wrong to us but we were concerned about changing the behaviour of an existing command. However, destroy jndi-binding only fails if all the servers fail. So it might be okay for us to have this return an error instead of success. [ Full content available at: https://github.com/apache/geode/pull/2918 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] dschneider-pivotal commented on pull request #2918: GEODE-6102: add gfsh destroy data-source
The "Change" message is a recent addition of the management team. It is not part of this pull request. But what we can do is change our implementation of updateConfigForGroup to return false in this case instead of true. [ Full content available at: https://github.com/apache/geode/pull/2918 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] dschneider-pivotal commented on pull request #2918: GEODE-6102: add gfsh destroy data-source
ditto [ Full content available at: https://github.com/apache/geode/pull/2918 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] dschneider-pivotal commented on pull request #2918: GEODE-6102: add gfsh destroy data-source
Throwing an exception works and is what create jndi-binding does. Also the EntityNotFoundException has special handling in gfsh to support "--ifexist" so I think we should stick with throwing the exception. You could submit a refactor request that all the gfsh commands that throw EntityNotFoundException be changed to instead return a CliFunctionResult [ Full content available at: https://github.com/apache/geode/pull/2918 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] dschneider-pivotal commented on pull request #2918: GEODE-6102: add gfsh destroy data-source
MANAGE was the permission on destroy jndi-binding which this command used as a template. I think MANAGE makes more sense than WRITE. I think WRITE has to do with changing user data (i.e. doing a put on a region). MANAGE allows you to change configuration but not user data. [ Full content available at: https://github.com/apache/geode/pull/2918 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
