You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by "pdxcodemonkey (GitHub)" <gi...@apache.org> on 2018/10/22 19:29:53 UTC
[GitHub] [geode-native] pdxcodemonkey opened pull request #382:
GEODE-4337: cpp example, function execution
- Adapted from 9.2 branch ExecuteFunctions quickstart.
- Minimal changes made, to best illustrate process to update from 9.2 to 10.0
[ Full content available at: https://github.com/apache/geode-native/pull/382 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] dgkimura commented on pull request #382:
GEODE-4337: cpp example, function execution
Posted by "dgkimura (GitHub)" <gi...@apache.org>.
Do we need this many puts/gets to demonstrate the example? Scrolling is hard..? Lol
[ Full content available at: https://github.com/apache/geode-native/pull/382 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #382:
GEODE-4337: cpp example, function execution
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
Fixed
[ Full content available at: https://github.com/apache/geode-native/pull/382 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] echobravopapa commented on pull request #382:
GEODE-4337: cpp example, function execution
Posted by "echobravopapa (GitHub)" <gi...@apache.org>.
copy/paste name needs update...
[ Full content available at: https://github.com/apache/geode-native/pull/382 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] dgkimura commented on pull request #382:
GEODE-4337: cpp example, function execution
Posted by "dgkimura (GitHub)" <gi...@apache.org>.
Is it just me or does something smell sorta funny about this interface...? Again out of scope and maybe good reason for it, but what if `getResult()` returned an empty container? Then seems like there wouldn't need to be an else?
[ Full content available at: https://github.com/apache/geode-native/pull/382 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] dgkimura commented on pull request #382:
GEODE-4337: cpp example, function execution
Posted by "dgkimura (GitHub)" <gi...@apache.org>.
Does the `close()` method have any real value now? Can it be removed from the api and just let the destructor do what it's supposed to do?
[ Full content available at: https://github.com/apache/geode-native/pull/382 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #382:
GEODE-4337: cpp example, function execution
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
Yes it would, but sadly we can't, or at least shouldn't. The purpose of doing it this way is to provide documentation in the code of what is required to update your app to work with the new version. The original quickstart code has this as a long function, so we've left it that way to allow for a simple diff. If we refactor into smaller methods, we'll lose the simplicity.
[ Full content available at: https://github.com/apache/geode-native/pull/382 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] echobravopapa commented on pull request #382:
GEODE-4337: cpp example, function execution
Posted by "echobravopapa (GitHub)" <gi...@apache.org>.
this example should be called `function-execution` as that is accurate and matches the docs too
[ Full content available at: https://github.com/apache/geode-native/pull/382 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #382:
GEODE-4337: cpp example, function execution
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
Fixed - added named constant EXAMPLE_ITEM_COUNT
[ Full content available at: https://github.com/apache/geode-native/pull/382 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] dgkimura commented on pull request #382:
GEODE-4337: cpp example, function execution
Posted by "dgkimura (GitHub)" <gi...@apache.org>.
`putFuncName ` seems unused? Was the intent to use it?
[ Full content available at: https://github.com/apache/geode-native/pull/382 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #382:
GEODE-4337: cpp example, function execution
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
If we get an empty container back, something is very wrong with how we've set up the cache and region. Likewise if we get nullptr back, although we do handle that eventuality. Think this one is out of scope for this exercise, though I value the feedback.
[ Full content available at: https://github.com/apache/geode-native/pull/382 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] dgkimura commented on pull request #382:
GEODE-4337: cpp example, function execution
Posted by "dgkimura (GitHub)" <gi...@apache.org>.
This function is almost 150 lines. Would it make sense to break it up and give useful function names that might help the reader understand the different stages of what's going on?
[ Full content available at: https://github.com/apache/geode-native/pull/382 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] dgkimura commented on pull request #382:
GEODE-4337: cpp example, function execution
Posted by "dgkimura (GitHub)" <gi...@apache.org>.
Since `cacheFactory` isn't used beyond this point, any reason not to remove it?
```cpp
auto cache = CacheFactory().set(...).create()
```
Seems like less moving parts may make it a little easier to follow.
[ Full content available at: https://github.com/apache/geode-native/pull/382 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] echobravopapa commented on pull request #382:
GEODE-4337: cpp example, function execution
Posted by "echobravopapa (GitHub)" <gi...@apache.org>.
update readme to match the example
[ Full content available at: https://github.com/apache/geode-native/pull/382 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] dgkimura commented on pull request #382:
GEODE-4337: cpp example, function execution
Posted by "dgkimura (GitHub)" <gi...@apache.org>.
Any reason why using raw `char` array instead of `std::string` or `std::stringstream`? I thought raw arrays is sorta frowned upon in modern C++.
[ Full content available at: https://github.com/apache/geode-native/pull/382 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] dgkimura commented on pull request #382:
GEODE-4337: cpp example, function execution
Posted by "dgkimura (GitHub)" <gi...@apache.org>.
Hmm... comment doesn't seem to match up and doesn't seem to add any value?
[ Full content available at: https://github.com/apache/geode-native/pull/382 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey closed pull request #382:
GEODE-4337: cpp example, function execution
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
[ pull request closed by pdxcodemonkey ]
[ Full content available at: https://github.com/apache/geode-native/pull/382 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #382:
GEODE-4337: cpp example, function execution
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
It's unused in the old quickstart code, though we expect the original intent was to use it. In the spirit of keeping the diff minimal and clear, we just removed the declaration of putFuncName.
[ Full content available at: https://github.com/apache/geode-native/pull/382 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] dgkimura commented on pull request #382:
GEODE-4337: cpp example, function execution
Posted by "dgkimura (GitHub)" <gi...@apache.org>.
Why 34? Magic numbers sometimes cause one to ponder, perhaps unnecessarily... ;)
[ Full content available at: https://github.com/apache/geode-native/pull/382 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #382:
GEODE-4337: cpp example, function execution
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
cache is declared in the scope of the try block, so close() can't be called in the catch block. OTOH, close() is called in the underlying CacheImpl dtor, so it's not necessary to explicitly call close() at all. We removed the close call, hope that makes things clearer.
[ Full content available at: https://github.com/apache/geode-native/pull/382 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #382:
GEODE-4337: cpp example, function execution
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
Yeah, it's not the cleanest code. We're gonna leave this in place as well in the spirit of keeping the diffs simple.
[ Full content available at: https://github.com/apache/geode-native/pull/382 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #382:
GEODE-4337: cpp example, function execution
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
Fixed
[ Full content available at: https://github.com/apache/geode-native/pull/382 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] dgkimura commented on pull request #382:
GEODE-4337: cpp example, function execution
Posted by "dgkimura (GitHub)" <gi...@apache.org>.
Same as above comment.
```cpp
auto pool = cache.getPoolManager()
.createFactory()
.setSubscriptionEnabled()
.addServer()
.addServer()
.create()
```
[ Full content available at: https://github.com/apache/geode-native/pull/382 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] dgkimura commented on pull request #382:
GEODE-4337: cpp example, function execution
Posted by "dgkimura (GitHub)" <gi...@apache.org>.
I think you might also consider using `std::transform` here
[ Full content available at: https://github.com/apache/geode-native/pull/382 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] dgkimura commented on pull request #382:
GEODE-4337: cpp example, function execution
Posted by "dgkimura (GitHub)" <gi...@apache.org>.
Is it best practice to close cache in exception block as well?
Likely out of scope, and I suspect that there's a reasonable answer for why not: have we considered closing cache in cache's destructor? It seems to me that this would be more inline with RAII.
[ Full content available at: https://github.com/apache/geode-native/pull/382 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #382:
GEODE-4337: cpp example, function execution
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
Removed
[ Full content available at: https://github.com/apache/geode-native/pull/382 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org