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

Posted to notifications@geode.apache.org by "BenjaminPerryRoss (GitHub)" <gi...@apache.org> on 2018/12/20 23:40:20 UTC

[GitHub] [geode] BenjaminPerryRoss opened pull request #3032: Feature/geode 6187

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/3032 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] dschneider-pivotal commented on pull request #3032: Feature/geode 6187

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

"id" should also be in MappingConstants

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

[GitHub] [geode] dschneider-pivotal commented on pull request #3032: Feature/geode 6187

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

I think a better name would be "isMappingSynchronous". Under the covers it looks at the region to figure this out but callers of this method are really asking about the mapping they pass as the first parameter.

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

[GitHub] [geode] gesterzhou closed pull request #3032: Feature/geode 6187

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

[ pull request closed by gesterzhou ]

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

[GitHub] [geode] dschneider-pivotal commented on issue #3032: Feature/geode 6187

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

dunit failures caused by this pr: DescribeMappingCommandDUnitTest, and CreateMappingCommandDUnitTest

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

[GitHub] [geode] jinmeiliao commented on pull request #3032: Feature/geode 6187

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

do not need to this. The rules already handles cleaning up.

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

[GitHub] [geode] dschneider-pivotal commented on pull request #3032: Feature/geode 6187

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

We should have a unit test for this new class. The main thing to test in that unit test is toData and fromData. I think the key thing to test is that the output of toData can be used as the input to fromData and the resulting changes made by fromData are equal to the original input of toData. This is not too hard. You can create a DataOutput that just writes to a byte array and then create a DataInput that reads from that byte array. You might want to add an equals method on this class or you could just ask info their maps are equal. Test toData with an empty map and a map with multiple entries.

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

[GitHub] [geode] dschneider-pivotal commented on pull request #3032: Feature/geode 6187

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

another drop of "myId"

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

[GitHub] [geode] dschneider-pivotal commented on pull request #3032: Feature/geode 6187

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

instead of using in.readInt keep your code "balanced" by using DataSerializer.readInteger

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

[GitHub] [geode] dschneider-pivotal commented on pull request #3032: Feature/geode 6187

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

another drop of "myId"

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

[GitHub] [geode] dschneider-pivotal commented on pull request #3032: Feature/geode 6187

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

add "id" here

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

[GitHub] [geode] dschneider-pivotal commented on pull request #3032: Feature/geode 6187

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

add test here for "id"

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

[GitHub] [geode] dschneider-pivotal commented on pull request #3032: Feature/geode 6187

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

I think it would be better to pass the region name in as a String instead of a RegionMapping. I see other methods on this class that take a region name like "getMappingForRegion".

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

[GitHub] [geode] davebarnes97 commented on issue #3032: Feature/geode 6187

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

Friendly suggestion: please add ticket summary in PR title. E.g. in this case "describe jdbc-mapping should show if mapping is synchronous or not". 

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

[GitHub] [geode] dschneider-pivotal commented on pull request #3032: Feature/geode 6187

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

add ID_NAME here

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

[GitHub] [geode] dschneider-pivotal commented on pull request #3032: Feature/geode 6187

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

make this final

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

[GitHub] [geode] jchen21 commented on issue #3032: GEODE-6187: describe jdbc-mapping should show if mapping is synchronous or not

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

> Friendly suggestion: please add ticket summary in PR title. E.g. in this case "describe jdbc-mapping should show if mapping is synchronous or not".

Thanks Dave. We just updated the PR title.

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

[GitHub] [geode] BenjaminPerryRoss commented on pull request #3032: Feature/geode 6187

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

We added a unit test for this class

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

[GitHub] [geode] dschneider-pivotal commented on pull request #3032: Feature/geode 6187

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

add "myId" here

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

[GitHub] [geode] dschneider-pivotal commented on pull request #3032: Feature/geode 6187

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

It looks like this line about "myId" was dropped. I think we want to keep it. Also lines 196 and 206 were dropped. This may have happened when resolving conflicts?

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