You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by "pivotal-jbarrett (GitHub)" <gi...@apache.org> on 2018/10/15 23:51:14 UTC
[GitHub] [geode-native] pivotal-jbarrett opened pull request #379: DO
NOT MERGE - for travis only
[ Full content available at: https://github.com/apache/geode-native/pull/379 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on pull request
#379: GEODE-2484: Removes most usage of ACE
Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.
Fixed
[ Full content available at: https://github.com/apache/geode-native/pull/379 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on pull request
#379: GEODE-2484: Removes most usage of ACE
Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.
More out of scope at this point.
[ Full content available at: https://github.com/apache/geode-native/pull/379 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #379:
GEODE-2484: Removes most usage of ACE
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
Ima just stop tagging these now, maybe we review all the usage of snprintf in a future JIRA item/PR.
[ Full content available at: https://github.com/apache/geode-native/pull/379 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #379:
GEODE-2484: Removes most usage of ACE
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
The various LOG* macros are variable argument, right? Why are we pre-formatting strings?
[ Full content available at: https://github.com/apache/geode-native/pull/379 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #379:
GEODE-2484: Removes most usage of ACE
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
This seems an unusual number to arrive at - why 2 threads per CPU, out of curiosity?
[ Full content available at: https://github.com/apache/geode-native/pull/379 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #379:
GEODE-2484: Removes most usage of ACE
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
As long as you're in here modifying stuff, can we give TSSDataOutput and s_tssDataOutput meaningful names? It's just a buffer pool, and we don't need a Hungarian prefix to know it's a static. Refactoring this has been on my to do list for a long time, and you're 90% of the way there, just need a name that's more meaningful than "foo".
[ Full content available at: https://github.com/apache/geode-native/pull/379 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on pull request
#379: GEODE-2484: Removes most usage of ACE
Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.
Sure, but way out of scope right now.
[ Full content available at: https://github.com/apache/geode-native/pull/379 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on pull request
#379: GEODE-2484: Removes most usage of ACE
Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.
We sure could, but it is really outside the scope.
[ Full content available at: https://github.com/apache/geode-native/pull/379 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #379:
GEODE-2484: Removes most usage of ACE
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
Is IntQueue doing us any good, or can we ditch it and just use std::deque<int32_t>?
[ Full content available at: https://github.com/apache/geode-native/pull/379 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #379:
GEODE-2484: Removes most usage of ACE
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
Ugh, magic number
[ Full content available at: https://github.com/apache/geode-native/pull/379 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #379:
GEODE-2484: Removes most usage of ACE
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
Yikes, this function gives me the willies. Just begging for an overflow bug or similar shenanigans. Is boost::format applicable here?
[ Full content available at: https://github.com/apache/geode-native/pull/379 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on pull request
#379: GEODE-2484: Removes most usage of ACE
Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.
This was a global search and replace. I will clean up this though.
[ Full content available at: https://github.com/apache/geode-native/pull/379 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #379:
GEODE-2484: Removes most usage of ACE
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
Don't you prefer `<inttypes>` for picking up the PR* definitions?
[ Full content available at: https://github.com/apache/geode-native/pull/379 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #379:
GEODE-2484: Removes most usage of ACE
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
Gah! Again with the Hungarian notation, let's stamp this out where we're already making changes.
[ Full content available at: https://github.com/apache/geode-native/pull/379 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #379:
GEODE-2484: Removes most usage of ACE
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
Again using x and y instead of / and \, either I don't get it or this is a mistake. Seems like a test somewhere ought to break if we end up with incorrect platform-specific separators in g_statFile...
[ Full content available at: https://github.com/apache/geode-native/pull/379 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on pull request
#379: GEODE-2484: Removes most usage of ACE
Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.
This was part of a global search and replace. I generally preserve cleanup of surrounding code for areas I am actually working in.
[ Full content available at: https://github.com/apache/geode-native/pull/379 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on pull request
#379: GEODE-2484: Removes most usage of ACE
Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.
I am sure there is. When I am addressing the last of the ACE stuff I will get to this.
[ Full content available at: https://github.com/apache/geode-native/pull/379 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #379:
GEODE-2484: Removes most usage of ACE
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
Use of raw char buffer again
[ Full content available at: https://github.com/apache/geode-native/pull/379 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #379:
GEODE-2484: Removes most usage of ACE
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
Okay, so now I see IntQueue appears to be a thread-safe wrapper around std::deque<T>. Perhaps we could give it a name that reflects that, like SynchronizedIntQueue or something?
[ Full content available at: https://github.com/apache/geode-native/pull/379 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on pull request
#379: GEODE-2484: Removes most usage of ACE
Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.
Oh good catch! Shoot, that's what I get for copy and paste and or tests are crap.
[ Full content available at: https://github.com/apache/geode-native/pull/379 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #379:
GEODE-2484: Removes most usage of ACE
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
Dunno what TSSUserAttributesWrapper is all about, but it's a cinch the name could be better, and the s_ can go from the static declaration.
[ Full content available at: https://github.com/apache/geode-native/pull/379 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on pull request
#379: GEODE-2484: Removes most usage of ACE
Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.
Opened https://issues.apache.org/jira/browse/GEODE-6005 to look into this big buffer thing. It seems like a pretty sketchy solution to resource management.
[ Full content available at: https://github.com/apache/geode-native/pull/379 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on pull request
#379: GEODE-2484: Removes most usage of ACE
Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.
Yes will do.
[ Full content available at: https://github.com/apache/geode-native/pull/379 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on pull request
#379: GEODE-2484: Removes most usage of ACE
Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.
Yeah, I pondered that too. Will do.
[ Full content available at: https://github.com/apache/geode-native/pull/379 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on pull request
#379: GEODE-2484: Removes most usage of ACE
Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.
Wasn't changing random headers in this PR.
[ Full content available at: https://github.com/apache/geode-native/pull/379 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on pull request
#379: GEODE-2484: Removes most usage of ACE
Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.
I didn't investigate it deeply to understand why they queue other than it encapsulates synchronization around the modifiers and accessors, std::dequeue does not provide protection. We would need a "synchronized_queue" wrapper template. Interestingly enough I am writing a `synchronized_map` wrapper template for protecting `std::*map` containers to replace `ACE_*MAP` implementations. This PR is focused on removing ACE and this `IntQueue` isn't ACE.
[ Full content available at: https://github.com/apache/geode-native/pull/379 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on pull request
#379: GEODE-2484: Removes most usage of ACE
Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.
Wasn't changing random headers in this PR, but yes that non `.h` versions for C++ are preferred.
[ Full content available at: https://github.com/apache/geode-native/pull/379 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #379:
GEODE-2484: Removes most usage of ACE
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
Surely there's a better definition available for separator characters?
[ Full content available at: https://github.com/apache/geode-native/pull/379 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on pull request
#379: GEODE-2484: Removes most usage of ACE
Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.
Not so magic anymore. It is now explicitly 100000us. ;)
[ Full content available at: https://github.com/apache/geode-native/pull/379 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on pull request
#379: GEODE-2484: Removes most usage of ACE
Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.
Fixed
[ Full content available at: https://github.com/apache/geode-native/pull/379 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #379:
GEODE-2484: Removes most usage of ACE
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
You may want to rebase to pick up the change that removes these constants from the code base.
[ Full content available at: https://github.com/apache/geode-native/pull/379 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on pull request
#379: GEODE-2484: Removes most usage of ACE
Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.
Your guess is as good as mine. As many of these are just global search and replace it is out of scope for me to address all of this.
[ Full content available at: https://github.com/apache/geode-native/pull/379 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on pull request
#379: GEODE-2484: Removes most usage of ACE
Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.
Your guess is as good as mine!
[ Full content available at: https://github.com/apache/geode-native/pull/379 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #379:
GEODE-2484: Removes most usage of ACE
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
Don't you prefer <inttypes> for picking up the PR* definitions?
[ Full content available at: https://github.com/apache/geode-native/pull/379 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #379:
GEODE-2484: Removes most usage of ACE
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
Why still use a raw buffer here? Can we just do std::string manipulation?
[ Full content available at: https://github.com/apache/geode-native/pull/379 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pivotal-jbarrett closed pull request #379:
GEODE-2484: Removes most usage of ACE
Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.
[ pull request closed by pivotal-jbarrett ]
[ Full content available at: https://github.com/apache/geode-native/pull/379 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #379:
GEODE-2484: Removes most usage of ACE
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
Dunno how many places this occurs, but the buf_size parameter to std::snprintf should be the same as the actual buffer size, _not_ one less. Which kinda obviates the problem with magic numbers all over the place in these
[ Full content available at: https://github.com/apache/geode-native/pull/379 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #379:
GEODE-2484: Removes most usage of ACE
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
Delete commented-out code
[ Full content available at: https://github.com/apache/geode-native/pull/379 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #379:
GEODE-2484: Removes most usage of ACE
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
This looks a little crazy. Can't we just include the "." in the snprintf call? And doesn't IllegalArgumentException have a ctor that takes a std::string? So we could just get rid of some of the work and `throw IllegalArgumentException(s + buf);`?
[ Full content available at: https://github.com/apache/geode-native/pull/379 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #379:
GEODE-2484: Removes most usage of ACE
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
What the heck? How do 'x' and 'y' relate to '/' and '\' here? Also you could lose the (unhelpful) comment...
[ Full content available at: https://github.com/apache/geode-native/pull/379 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #379:
GEODE-2484: Removes most usage of ACE
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
Gratuitous Hungarian notation again...
[ Full content available at: https://github.com/apache/geode-native/pull/379 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #379:
GEODE-2484: Removes most usage of ACE
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
Duplicate namespace comment, looks like
[ Full content available at: https://github.com/apache/geode-native/pull/379 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #379:
GEODE-2484: Removes most usage of ACE
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
GEODE-6007 in Jira
[ Full content available at: https://github.com/apache/geode-native/pull/379 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on pull request
#379: GEODE-2484: Removes most usage of ACE
Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.
Dude, I am not spell checking old comments.
[ Full content available at: https://github.com/apache/geode-native/pull/379 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on pull request
#379: GEODE-2484: Removes most usage of ACE
Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.
Yup, not tackling bad logging messages in this PR.
[ Full content available at: https://github.com/apache/geode-native/pull/379 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #379:
GEODE-2484: Removes most usage of ACE
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
More raw buffer shenanigans
[ Full content available at: https://github.com/apache/geode-native/pull/379 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #379:
GEODE-2484: Removes most usage of ACE
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
buckets?
[ Full content available at: https://github.com/apache/geode-native/pull/379 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org