You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by PivotalSarge <gi...@git.apache.org> on 2017/03/07 19:28:10 UTC
[GitHub] geode-native pull request #48: GEODE-2578: Remove 64 KiB limit on query stri...
GitHub user PivotalSarge opened a pull request:
https://github.com/apache/geode-native/pull/48
GEODE-2578: Remove 64 KiB limit on query strings.
- Remove artificial cap of 65535 for query string
length by using 32 bits for the length of query
strings in DataOutput::writeFullUTF().
- Rename parameter to TcrMessage::writeStringPart()
whose name is misleading due to copy-and-paste.
- Add three unit tests for query-related messages:
* testConstructorEXECUTECQ_MSG_TYPE
* testConstructorWithGinormousQueryEXECUTECQ_MSG_TYPE
* testConstructorEXECUTECQ_WITH_IR_MSG_TYPE
- Fix the cursor advance tests to not depend on
specific values for buffer length and not to
test remaining buffer length.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/PivotalSarge/geode-native feature/GEODE-2578
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/geode-native/pull/48.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #48
----
commit 128ab410d3aaae69186c0a41ed3a1310ef982382
Author: Sarge <md...@pivotal.io>
Date: 2017-03-07T19:26:53Z
GEODE-2578: Remove 64 KiB limit on query strings.
- Remove artificial cap of 65535 for query string
length by using 32 bits for the length of query
strings in DataOutput::writeFullUTF().
- Rename parameter to TcrMessage::writeStringPart()
whose name is misleading due to copy-and-paste.
- Add three unit tests for query-related messages:
* testConstructorEXECUTECQ_MSG_TYPE
* testConstructorWithGinormousQueryEXECUTECQ_MSG_TYPE
* testConstructorEXECUTECQ_WITH_IR_MSG_TYPE
- Fix the cursor advance tests to not depend on
specific values for buffer length and not to
test remaining buffer length.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode-native pull request #48: GEODE-2578: Remove 64 KiB limit on query stri...
Posted by echobravopapa <gi...@git.apache.org>.
Github user echobravopapa commented on a diff in the pull request:
https://github.com/apache/geode-native/pull/48#discussion_r104768254
--- Diff: src/cppcache/test/TcrMessage_unittest.cpp ---
@@ -559,3 +560,66 @@ TEST_F(TcrMessageTest, testConstructorEXECUTE_FUNCTION) {
"75746546756E6374696F6E0000000301570000",
testMessage);
}
+
+TEST_F(TcrMessageTest, testConstructorEXECUTECQ_MSG_TYPE) {
+ CacheablePtr myCacheablePtr(CacheableString::createDeserializable());
+
+ TcrMessageExecuteCq testMessage("ExecuteCQ", "select * from /somewhere",
+ CqState::RUNNING, false,
+ static_cast<ThinClientBaseDM *>(NULL));
+
+ EXPECT_EQ(TcrMessage::EXECUTECQ_MSG_TYPE, testMessage.getMessageType());
+
+ EXPECT_MESSAGE_EQ(
+ "0000002A0000004000000005FFFFFFFF0000000009004578656375746543510000001800"
+ "73656C656374202A2066726F6D202F736F6D657768657265000000040000000001000000"
+ "010000000000010001",
+ testMessage);
+}
+
+TEST_F(TcrMessageTest, testConstructorWithGinormousQueryEXECUTECQ_MSG_TYPE) {
--- End diff --
I'm not certain that this query's magnitude qualifies for ginormity... maybe we just call it test*64KiBQuery* - lest we set the bar low for ginormous...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode-native pull request #48: GEODE-2578: Remove 64 KiB limit on query stri...
Posted by PivotalSarge <gi...@git.apache.org>.
Github user PivotalSarge commented on a diff in the pull request:
https://github.com/apache/geode-native/pull/48#discussion_r104769260
--- Diff: src/cppcache/test/TcrMessage_unittest.cpp ---
@@ -559,3 +560,66 @@ TEST_F(TcrMessageTest, testConstructorEXECUTE_FUNCTION) {
"75746546756E6374696F6E0000000301570000",
testMessage);
}
+
+TEST_F(TcrMessageTest, testConstructorEXECUTECQ_MSG_TYPE) {
+ CacheablePtr myCacheablePtr(CacheableString::createDeserializable());
+
+ TcrMessageExecuteCq testMessage("ExecuteCQ", "select * from /somewhere",
+ CqState::RUNNING, false,
+ static_cast<ThinClientBaseDM *>(NULL));
+
+ EXPECT_EQ(TcrMessage::EXECUTECQ_MSG_TYPE, testMessage.getMessageType());
+
+ EXPECT_MESSAGE_EQ(
+ "0000002A0000004000000005FFFFFFFF0000000009004578656375746543510000001800"
+ "73656C656374202A2066726F6D202F736F6D657768657265000000040000000001000000"
+ "010000000000010001",
+ testMessage);
+}
+
+TEST_F(TcrMessageTest, testConstructorWithGinormousQueryEXECUTECQ_MSG_TYPE) {
--- End diff --
I'm unaware of the SI definition of ginormous...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode-native pull request #48: GEODE-2578: Remove 64 KiB limit on query stri...
Posted by PivotalSarge <gi...@git.apache.org>.
Github user PivotalSarge commented on a diff in the pull request:
https://github.com/apache/geode-native/pull/48#discussion_r104768886
--- Diff: src/cppcache/include/geode/DataOutput.hpp ---
@@ -378,9 +378,8 @@ class CPPCACHE_EXPORT DataOutput {
*/
inline void writeFullUTF(const char* value, uint32_t length = 0) {
if (value != NULL) {
- int32_t len = getEncodedLength(value, length);
- uint16_t encodedLen = static_cast<uint16_t>(len > 0xFFFF ? 0xFFFF : len);
--- End diff --
If the query string happened to be longer that 0xFFFF, it would be artificially truncated which often led to errors on the server side.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode-native pull request #48: GEODE-2578: Remove 64 KiB limit on query stri...
Posted by echobravopapa <gi...@git.apache.org>.
Github user echobravopapa commented on a diff in the pull request:
https://github.com/apache/geode-native/pull/48#discussion_r105204008
--- Diff: src/cppcache/include/geode/DataOutput.hpp ---
@@ -378,9 +378,8 @@ class CPPCACHE_EXPORT DataOutput {
*/
inline void writeFullUTF(const char* value, uint32_t length = 0) {
--- End diff --
ICKY, sounds like a separable refactoring chore...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode-native pull request #48: GEODE-2578: Remove 64 KiB limit on query stri...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/geode-native/pull/48
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode-native pull request #48: GEODE-2578: Remove 64 KiB limit on query stri...
Posted by echobravopapa <gi...@git.apache.org>.
Github user echobravopapa commented on a diff in the pull request:
https://github.com/apache/geode-native/pull/48#discussion_r104759431
--- Diff: src/cppcache/src/TcrMessage.hpp ---
@@ -555,7 +555,7 @@ class CPPCACHE_EXPORT TcrMessage {
const VectorOfCacheableKey* getAllKeyList = NULL);
void writeHeader(uint32_t msgType, uint32_t numOfParts);
void writeRegionPart(const std::string& regionName);
- void writeStringPart(const std::string& regionName);
+ void writeStringPart(const std::string& str);
--- End diff --
the aforementioned copy/pasting...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode-native pull request #48: GEODE-2578: Remove 64 KiB limit on query stri...
Posted by echobravopapa <gi...@git.apache.org>.
Github user echobravopapa commented on a diff in the pull request:
https://github.com/apache/geode-native/pull/48#discussion_r104766552
--- Diff: src/cppcache/test/DataOutputTest.cpp ---
@@ -305,15 +305,9 @@ TEST_F(DataOutputTest, TestCursorAdvance) {
"001B596F7520686164206D65206174206D65617420746F726E61646F2E",
dataOutput.getByteArray());
- EXPECT_EQ((2 + 27), dataOutput.getBufferLength());
-
- // buffers are pre-allocated 8k and have 2 bytes to hold the data length
- EXPECT_EQ(((8 * 1024) - (2 + 27)), dataOutput.getRemainingBufferLength());
--- End diff --
What led you to removing the call to getRemainingBufferLength()?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode-native pull request #48: GEODE-2578: Remove 64 KiB limit on query stri...
Posted by echobravopapa <gi...@git.apache.org>.
Github user echobravopapa commented on a diff in the pull request:
https://github.com/apache/geode-native/pull/48#discussion_r104766406
--- Diff: src/cppcache/include/geode/DataOutput.hpp ---
@@ -378,9 +378,8 @@ class CPPCACHE_EXPORT DataOutput {
*/
inline void writeFullUTF(const char* value, uint32_t length = 0) {
if (value != NULL) {
- int32_t len = getEncodedLength(value, length);
- uint16_t encodedLen = static_cast<uint16_t>(len > 0xFFFF ? 0xFFFF : len);
--- End diff --
what was the corner case impact of this line?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode-native pull request #48: GEODE-2578: Remove 64 KiB limit on query stri...
Posted by echobravopapa <gi...@git.apache.org>.
Github user echobravopapa commented on a diff in the pull request:
https://github.com/apache/geode-native/pull/48#discussion_r104759602
--- Diff: src/cppcache/src/TcrMessage.cpp ---
@@ -756,8 +756,8 @@ void TcrMessage::writeRegionPart(const std::string& regionName) {
m_request->writeBytesOnly((int8_t*)regionName.c_str(), len);
}
-void TcrMessage::writeStringPart(const std::string& regionName) {
- m_request->writeFullUTF(regionName.c_str());
+void TcrMessage::writeStringPart(const std::string& str) {
--- End diff --
here as well, much more intuitive var name
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode-native pull request #48: GEODE-2578: Remove 64 KiB limit on query stri...
Posted by pivotal-jbarrett <gi...@git.apache.org>.
Github user pivotal-jbarrett commented on a diff in the pull request:
https://github.com/apache/geode-native/pull/48#discussion_r104816808
--- Diff: src/cppcache/include/geode/DataOutput.hpp ---
@@ -378,9 +378,8 @@ class CPPCACHE_EXPORT DataOutput {
*/
inline void writeFullUTF(const char* value, uint32_t length = 0) {
--- End diff --
If we are being sticklers and consistent this method has no business here. It is only used by the Message writeStringPart and should really be implemented there.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode-native pull request #48: GEODE-2578: Remove 64 KiB limit on query stri...
Posted by PivotalSarge <gi...@git.apache.org>.
Github user PivotalSarge commented on a diff in the pull request:
https://github.com/apache/geode-native/pull/48#discussion_r104769132
--- Diff: src/cppcache/test/DataOutputTest.cpp ---
@@ -305,15 +305,9 @@ TEST_F(DataOutputTest, TestCursorAdvance) {
"001B596F7520686164206D65206174206D65617420746F726E61646F2E",
dataOutput.getByteArray());
- EXPECT_EQ((2 + 27), dataOutput.getBufferLength());
-
- // buffers are pre-allocated 8k and have 2 bytes to hold the data length
- EXPECT_EQ(((8 * 1024) - (2 + 27)), dataOutput.getRemainingBufferLength());
--- End diff --
getRemainingBufferLength() is the call that exhibited less-than-obvious behavior. In particular, it appears to be affected by some global state. Its return value was not predictable enough to be tested in that manner.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode-native pull request #48: GEODE-2578: Remove 64 KiB limit on query stri...
Posted by PivotalSarge <gi...@git.apache.org>.
Github user PivotalSarge commented on a diff in the pull request:
https://github.com/apache/geode-native/pull/48#discussion_r104836958
--- Diff: src/cppcache/include/geode/DataOutput.hpp ---
@@ -378,9 +378,8 @@ class CPPCACHE_EXPORT DataOutput {
*/
inline void writeFullUTF(const char* value, uint32_t length = 0) {
--- End diff --
I agree and tried to move it to TcrMessage. However, its implementation depends on private members of DataOutput.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---