You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by GitBox <gi...@apache.org> on 2020/05/16 16:45:18 UTC
[GitHub] [ignite] ashu210890 opened a new pull request #7808: IGNITE-7153: Fix parser to handle incomplete Redis packet
ashu210890 opened a new pull request #7808:
URL: https://github.com/apache/ignite/pull/7808
Add validation method to check packet completeness
Add logic to store incomplete packet temporarily and assemble them until
the final CRLF is seen.
Implement test cases for data = 8k, 10k and 16k
Thank you for submitting the pull request to the Apache Ignite.
In order to streamline the review of the contribution
we ask you to ensure the following steps have been taken:
### The Contribution Checklist
- [ ] There is a single JIRA ticket related to the pull request.
- [ ] The web-link to the pull request is attached to the JIRA ticket.
- [ ] The JIRA ticket has the _Patch Available_ state.
- [ ] The pull request body describes changes that have been made.
The description explains _WHAT_ and _WHY_ was made instead of _HOW_.
- [ ] The pull request title is treated as the final commit message.
The following pattern must be used: `IGNITE-12407: Add Cluster API support to Java thin client`
- [ ] A reviewer has been mentioned through the JIRA comments
(see [the Maintainers list](https://cwiki.apache.org/confluence/display/IGNITE/How+to+Contribute#HowtoContribute-ReviewProcessandMaintainers))
- [ ] The pull request has been checked by the Teamcity Bot and
the `green visa` attached to the JIRA ticket (see [TC.Bot: Check PR](https://mtcga.gridgain.com/prs.html))
### Notes
- [How to Contribute](https://cwiki.apache.org/confluence/display/IGNITE/How+to+Contribute)
- [Coding abbreviation rules](https://cwiki.apache.org/confluence/display/IGNITE/Abbreviation+Rules)
- [Coding Guidelines](https://cwiki.apache.org/confluence/display/IGNITE/Coding+Guidelines)
- [Apache Ignite Teamcity Bot](https://cwiki.apache.org/confluence/display/IGNITE/Apache+Ignite+Teamcity+Bot)
If you need any help, please email dev@ignite.apache.org or ask anу advice on http://asf.slack.com _#ignite_ channel.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] dmagda commented on a change in pull request #7808: IGNITE-7153: Fix parser to handle incomplete Redis packet
Posted by GitBox <gi...@apache.org>.
dmagda commented on a change in pull request #7808:
URL: https://github.com/apache/ignite/pull/7808#discussion_r428315712
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/rest/protocols/tcp/GridTcpRestParser.java
##########
@@ -246,6 +246,45 @@ else if (msg instanceof GridRouterRequest) {
}
}
+ /**
+ * Parses redis protocol message.
+ *
+ * @param ses Session.
+ * @param buf Buffer containing not parsed bytes.
+ * @param state Current parser state.
+ * @return Parsed packet.s
+ * @throws IOException If packet cannot be parsed.
+ * @throws IgniteCheckedException If deserialization error occurred.
+ */
+ private GridClientMessage parseRedisPacket(GridNioSession ses, ByteBuffer buf, ParserState state)
Review comment:
Wouldn't it be better to move this method to GridRedisProtocolParser.java class and renaming it to "readPacket"? This would also allow us to use "private" visibility level for the "validate.." set of the new methods
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/rest/protocols/tcp/redis/GridRedisProtocolParser.java
##########
@@ -112,6 +115,60 @@ public static String readBulkStr(ByteBuffer buf) throws IgniteCheckedException {
return new String(bulkStr);
}
+ /*
+ * A validation method to check packet completeness.
+ * return true if and only if
+ * 1. First byte is ARRAY (43)
+ * 2. Last two bytes are CR(13) LF(10)
+ *
+ * Otherwise, return false representing this is an incomplete packet with three possible scenarios:
+ * 1. A beginning packet with leading ARRAY byte
+ * 2. A continual packet with ending CRLF bytes.
+ * 3. A continual packet with neither conditions above.
+ */
+ public static boolean validatePacket(ByteBuffer buf) {
Review comment:
I would suggest naming the method "isCompletePacket"
##########
File path: modules/clients/src/test/java/org/apache/ignite/internal/processors/rest/protocols/tcp/redis/RedisProtocolConnectSelfTest.java
##########
@@ -74,4 +75,21 @@ public void testSelect() throws Exception {
Assert.assertEquals("v0", jedis.get("k0"));
}
}
+
+ //IGNITE-7153
Review comment:
Please remove the mentioning of "IGNITE-7153". Instead, use the text below as a method description
```
/**
* @throws Exception If failed.
*/
@Test
```
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/rest/protocols/tcp/redis/GridRedisProtocolParser.java
##########
@@ -112,6 +115,60 @@ public static String readBulkStr(ByteBuffer buf) throws IgniteCheckedException {
return new String(bulkStr);
}
+ /*
+ * A validation method to check packet completeness.
+ * return true if and only if
+ * 1. First byte is ARRAY (43)
+ * 2. Last two bytes are CR(13) LF(10)
+ *
+ * Otherwise, return false representing this is an incomplete packet with three possible scenarios:
+ * 1. A beginning packet with leading ARRAY byte
+ * 2. A continual packet with ending CRLF bytes.
+ * 3. A continual packet with neither conditions above.
+ */
+ public static boolean validatePacket(ByteBuffer buf) {
+ return validatePacketHeader(buf) && validatePacketFooter(buf);
+ }
+
+ public static boolean validatePacketHeader(ByteBuffer buf) {
+ boolean result = true;
+
+ //mark at initial position
+ buf.mark();
+
+ if(buf.get() != ARRAY) {
+ result = false;
+ }
+
+ //reset to initial position
+ buf.reset();
+
+ return result;
+ }
+
+ public static boolean validatePacketFooter(ByteBuffer buf) {
+ boolean result = true;
+
+ //mark at initial position
+ buf.mark();
+
Review comment:
Extra space, please remove. Check guidelines for reference: https://cwiki.apache.org/confluence/display/IGNITE/Coding+Guidelines#CodingGuidelines-Whitespacesandemptylines
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org