You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by "dschneider-pivotal (GitHub)" <gi...@apache.org> on 2018/10/31 22:49:40 UTC
[GitHub] [geode] dschneider-pivotal opened pull request #2758:
GEODE-5951: add a create data-source gfsh command
The new "create data-source" command is a replacement for "create jndi-binding". It has fewer options so will be easier for users to understand. Once it is done parsing its options it ends up using the same internal code as "create jndi-binding" to create the actual data-source.
Co-authored-by: @monkeyherder
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/2758 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jchen21 commented on pull request #2758: GEODE-5951:
add a create data-source gfsh command
Posted by "jchen21 (GitHub)" <gi...@apache.org>.
There is no value for `--pooled`. Is it expected?
[ Full content available at: https://github.com/apache/geode/pull/2758 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] agingade commented on pull request #2758:
GEODE-5951: add a create data-source gfsh command
Posted by "agingade (GitHub)" <gi...@apache.org>.
Can we verify the name of the data source; to make sure its the one that got created.
[ Full content available at: https://github.com/apache/geode/pull/2758 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] dschneider-pivotal commented on pull request #2758:
GEODE-5951: add a create data-source gfsh command
Posted by "dschneider-pivotal (GitHub)" <gi...@apache.org>.
done
[ Full content available at: https://github.com/apache/geode/pull/2758 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] dschneider-pivotal commented on pull request #2758:
GEODE-5951: add a create data-source gfsh command
Posted by "dschneider-pivotal (GitHub)" <gi...@apache.org>.
done
[ Full content available at: https://github.com/apache/geode/pull/2758 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] dschneider-pivotal commented on pull request #2758:
GEODE-5951: add a create data-source gfsh command
Posted by "dschneider-pivotal (GitHub)" <gi...@apache.org>.
done
[ Full content available at: https://github.com/apache/geode/pull/2758 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jchen21 commented on pull request #2758: GEODE-5951:
add a create data-source gfsh command
Posted by "jchen21 (GitHub)" <gi...@apache.org>.
Is it better to rename `DataSourceProperty` to `PoolProperty`? Because the variable is called `poolProperties`?
[ Full content available at: https://github.com/apache/geode/pull/2758 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] dschneider-pivotal commented on pull request #2758:
GEODE-5951: add a create data-source gfsh command
Posted by "dschneider-pivotal (GitHub)" <gi...@apache.org>.
yes, but you should also be able to explicitly set it to false. In this context I think it is better not to depend on the default.
[ Full content available at: https://github.com/apache/geode/pull/2758 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jinmeiliao commented on pull request #2758:
GEODE-5951: add a create data-source gfsh command
Posted by "jinmeiliao (GitHub)" <gi...@apache.org>.
I would prefer not to add this flag here, since it's only used in this specific command. These object are intended to be simple POJOs that represents the configuration attributes of the object. this flag seems to have dirtied this object. Instead, I would pass this flag as an addition argument to the CreateJndiBindingFunction. See CreateDiskstoreFunction as an example of how to pass multiple arguments to the function.
[ Full content available at: https://github.com/apache/geode/pull/2758 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jchen21 commented on pull request #2758: GEODE-5951:
add a create data-source gfsh command
Posted by "jchen21 (GitHub)" <gi...@apache.org>.
Is `false` the default value for `--pooled`?
[ Full content available at: https://github.com/apache/geode/pull/2758 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] dschneider-pivotal commented on pull request #2758:
GEODE-5951: add a create data-source gfsh command
Posted by "dschneider-pivotal (GitHub)" <gi...@apache.org>.
done and done
[ Full content available at: https://github.com/apache/geode/pull/2758 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jinmeiliao commented on pull request #2758:
GEODE-5951: add a create data-source gfsh command
Posted by "jinmeiliao (GitHub)" <gi...@apache.org>.
use String.format instead
[ Full content available at: https://github.com/apache/geode/pull/2758 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] dschneider-pivotal commented on pull request #2758:
GEODE-5951: add a create data-source gfsh command
Posted by "dschneider-pivotal (GitHub)" <gi...@apache.org>.
Let me know if some change is still needed on this part of your review.
[ Full content available at: https://github.com/apache/geode/pull/2758 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] dschneider-pivotal commented on pull request #2758:
GEODE-5951: add a create data-source gfsh command
Posted by "dschneider-pivotal (GitHub)" <gi...@apache.org>.
done
[ Full content available at: https://github.com/apache/geode/pull/2758 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jchen21 commented on pull request #2758: GEODE-5951:
add a create data-source gfsh command
Posted by "jchen21 (GitHub)" <gi...@apache.org>.
The servers arguments are missing.
[ Full content available at: https://github.com/apache/geode/pull/2758 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jinmeiliao commented on pull request #2758:
GEODE-5951: add a create data-source gfsh command
Posted by "jinmeiliao (GitHub)" <gi...@apache.org>.
let's not add to this class anymore. Put it in where the string is used.
[ Full content available at: https://github.com/apache/geode/pull/2758 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jchen21 commented on pull request #2758: GEODE-5951:
add a create data-source gfsh command
Posted by "jchen21 (GitHub)" <gi...@apache.org>.
Shall we initialized it? I don't see a `false` value is assigned in this pull request.
[ Full content available at: https://github.com/apache/geode/pull/2758 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] dschneider-pivotal commented on pull request #2758:
GEODE-5951: add a create data-source gfsh command
Posted by "dschneider-pivotal (GitHub)" <gi...@apache.org>.
done
[ Full content available at: https://github.com/apache/geode/pull/2758 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] dschneider-pivotal closed pull request #2758:
GEODE-5951: add a create data-source gfsh command
Posted by "dschneider-pivotal (GitHub)" <gi...@apache.org>.
[ pull request closed by dschneider-pivotal ]
[ Full content available at: https://github.com/apache/geode/pull/2758 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] dschneider-pivotal commented on pull request #2758:
GEODE-5951: add a create data-source gfsh command
Posted by "dschneider-pivotal (GitHub)" <gi...@apache.org>.
done
[ Full content available at: https://github.com/apache/geode/pull/2758 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jinmeiliao commented on pull request #2758:
GEODE-5951: add a create data-source gfsh command
Posted by "jinmeiliao (GitHub)" <gi...@apache.org>.
instead of dealing with the raw xml string, you can use the CacheConfig representation of the xml and verify the content that way.
CacheConfig cluster = ccService.getCacheConfig("cluster");
JndiBiding = cluster.getJndiBindings().get(0);
// verify the values of the jndiBing
[ Full content available at: https://github.com/apache/geode/pull/2758 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] dschneider-pivotal commented on pull request #2758:
GEODE-5951: add a create data-source gfsh command
Posted by "dschneider-pivotal (GitHub)" <gi...@apache.org>.
Yes, one of the ways you can use boolean options in gfsh is leave off the =X part. On this option, --pooled is the same as --pooled=true
[ Full content available at: https://github.com/apache/geode/pull/2758 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jinmeiliao commented on pull request #2758:
GEODE-5951: add a create data-source gfsh command
Posted by "jinmeiliao (GitHub)" <gi...@apache.org>.
and use the non-deprecated form of this function
[ Full content available at: https://github.com/apache/geode/pull/2758 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] dschneider-pivotal commented on issue #2758:
GEODE-5951: add a create data-source gfsh command
Posted by "dschneider-pivotal (GitHub)" <gi...@apache.org>.
the PRClientServerRegionFunctionExecutionDUnitTest.testBug43430 failure is a know issue
[ Full content available at: https://github.com/apache/geode/pull/2758 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] dschneider-pivotal commented on pull request #2758:
GEODE-5951: add a create data-source gfsh command
Posted by "dschneider-pivotal (GitHub)" <gi...@apache.org>.
the CreateDataSourceCommandInterceptor validates that "pooled" is true if --pool-properties is used. Is that what you mean by "verify"?
[ Full content available at: https://github.com/apache/geode/pull/2758 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] dschneider-pivotal commented on pull request #2758:
GEODE-5951: add a create data-source gfsh command
Posted by "dschneider-pivotal (GitHub)" <gi...@apache.org>.
false is the java default for a boolean instance variable. Also I think this is going away based in Jinmai's review.
[ Full content available at: https://github.com/apache/geode/pull/2758 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] dschneider-pivotal commented on pull request #2758:
GEODE-5951: add a create data-source gfsh command
Posted by "dschneider-pivotal (GitHub)" <gi...@apache.org>.
done
[ Full content available at: https://github.com/apache/geode/pull/2758 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jinmeiliao commented on pull request #2758:
GEODE-5951: add a create data-source gfsh command
Posted by "jinmeiliao (GitHub)" <gi...@apache.org>.
would be good to verify that "pooled" value is true as well. This is make sure we've added the "specifiedDefaultValue" for the boolean option.
[ Full content available at: https://github.com/apache/geode/pull/2758 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jinmeiliao commented on pull request #2758:
GEODE-5951: add a create data-source gfsh command
Posted by "jinmeiliao (GitHub)" <gi...@apache.org>.
I would prefer not to add this flag here, since it's only used in this specific command. It seems to have dirtied this object. Instead, I would pass this flag as an addition argument to the CreateJndiBindingFunction. See CreateDiskstoreFunction as an example of how to pass multiple arguments to the function.
[ Full content available at: https://github.com/apache/geode/pull/2758 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] dschneider-pivotal commented on issue #2758:
GEODE-5951: add a create data-source gfsh command
Posted by "dschneider-pivotal (GitHub)" <gi...@apache.org>.
The dunit failure EvictionDUnitTest > testDummyInlineNCentralizedEviction is due to a recent checkin of the EvictionDUnitTest made in another pull request. So I think this pull request is okay to merge once reviewed.
[ Full content available at: https://github.com/apache/geode/pull/2758 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jchen21 commented on pull request #2758: GEODE-5951:
add a create data-source gfsh command
Posted by "jchen21 (GitHub)" <gi...@apache.org>.
server names are missing.
[ Full content available at: https://github.com/apache/geode/pull/2758 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org