You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2021/09/28 23:32:46 UTC
[GitHub] [nifi] greyp9 opened a new pull request #5420: NIFI-9207 - limit read size, Distributed Cache Servers
greyp9 opened a new pull request #5420:
URL: https://github.com/apache/nifi/pull/5420
#### Description of PR
Add max read size property to distributed cache servers (Set, Map) to validate incoming cache client calls. Add unit tests to exercise default limits. Explicit client handling of socket exceptions (handle server channel disconnect).
In order to streamline the review of the contribution we ask you to ensure the following steps have been taken:
### For all changes:
- [x] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [x] Does your PR title start with **NIFI-XXXX** where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [x] Has your PR been rebased against the latest commit within the target branch (typically `main`)?
- [x] Is your initial contribution a single, squashed commit? _Additional commits in response to PR reviewer feedback should be made on this branch and pushed to allow change tracking. Do not `squash` or use `--force` when pushing to allow for clean monitoring of changes._
### For code changes:
- [x] Have you ensured that the full suite of tests is executed via `mvn -Pcontrib-check clean install` at the root `nifi` folder?
- [x] Have you written or updated unit tests to verify your changes?
- [x] Have you verified that the full build is successful on JDK 8?
- [x] Have you verified that the full build is successful on JDK 11?
- [ ] 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)?
- [ ] If applicable, have you updated the `LICENSE` file, including the main `LICENSE` file under `nifi-assembly`?
- [ ] If applicable, have you updated the `NOTICE` file, including the main `NOTICE` file found under `nifi-assembly`?
- [ ] If adding new Properties, have you added `.displayName` in addition to .name (programmatic access) for each of the new properties?
### For documentation related changes:
- [ ] Have you ensured that format looks appropriate for the output in which it is rendered?
### Note:
Please ensure that once the PR is submitted, you check GitHub Actions CI for build issues and submit an update to your PR as soon as possible.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] gresockj commented on a change in pull request #5420: NIFI-9207 - limit read size, Distributed Cache Servers
Posted by GitBox <gi...@apache.org>.
gresockj commented on a change in pull request #5420:
URL: https://github.com/apache/nifi/pull/5420#discussion_r718375440
##########
File path: nifi-nar-bundles/nifi-standard-services/nifi-distributed-cache-services-bundle/nifi-distributed-cache-client-service/src/main/java/org/apache/nifi/distributed/cache/client/CacheClientRequestHandler.java
##########
@@ -64,6 +64,18 @@ public void channelReadComplete(ChannelHandlerContext ctx) throws IOException {
}
}
+ @Override
+ public void channelUnregistered(ChannelHandlerContext ctx) {
Review comment:
Consider `final` argument here
##########
File path: nifi-nar-bundles/nifi-standard-services/nifi-distributed-cache-services-bundle/nifi-distributed-cache-server/src/main/java/org/apache/nifi/distributed/cache/server/map/DistributedMapCacheServer.java
##########
@@ -67,14 +68,14 @@ protected CacheServer createCacheServer(final ConfigurationContext context) {
try {
final File persistenceDir = persistencePath == null ? null : new File(persistencePath);
- return createMapCacheServer(port, maxSize, sslContext, evictionPolicy, persistenceDir);
+ return createMapCacheServer(port, maxSize, sslContext, evictionPolicy, persistenceDir, maxReadSize);
} catch (final Exception e) {
throw new RuntimeException(e);
}
}
- protected MapCacheServer createMapCacheServer(int port, int maxSize, SSLContext sslContext, EvictionPolicy evictionPolicy, File persistenceDir) throws IOException {
- return new MapCacheServer(getIdentifier(), sslContext, port, maxSize, evictionPolicy, persistenceDir);
+ protected MapCacheServer createMapCacheServer(int port, int maxSize, SSLContext sslContext, EvictionPolicy evictionPolicy, File persistenceDir, int maxReadSize) throws IOException {
Review comment:
Recommend `final` arguments
##########
File path: nifi-nar-bundles/nifi-standard-services/nifi-distributed-cache-services-bundle/nifi-distributed-cache-server/src/main/java/org/apache/nifi/distributed/cache/server/DistributedSetCacheServer.java
##########
@@ -35,6 +35,7 @@ protected CacheServer createCacheServer(final ConfigurationContext context) {
final SSLContextService sslContextService = context.getProperty(SSL_CONTEXT_SERVICE).asControllerService(SSLContextService.class);
final int maxSize = context.getProperty(MAX_CACHE_ENTRIES).asInteger();
final String evictionPolicyName = context.getProperty(EVICTION_POLICY).getValue();
+ final int maxReadSize = context.getProperty(MAX_READ_SIZE).asInteger();
Review comment:
If we go with the Data Size Validator, should `maxReadSize` be a `long`? Anything over ~2.14 GB is over `Integer.MAX_SIZE`. Or if we want to limit the max data size, perhaps additional custom validation is needed.
##########
File path: nifi-nar-bundles/nifi-standard-services/nifi-distributed-cache-services-bundle/nifi-distributed-cache-client-service/src/main/java/org/apache/nifi/distributed/cache/client/CacheClientRequestHandler.java
##########
@@ -64,6 +64,18 @@ public void channelReadComplete(ChannelHandlerContext ctx) throws IOException {
}
}
+ @Override
+ public void channelUnregistered(ChannelHandlerContext ctx) {
+ if (!inboundAdapter.isComplete()) {
+ channelPromise.setFailure(new IOException("channelUnregistered during request - " + ctx.channel().toString()));
+ }
+ }
+
+ @Override
+ public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) {
Review comment:
Consider `final` arguments
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on pull request #5420: NIFI-9207 - limit read size, Distributed Cache Servers
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on pull request #5420:
URL: https://github.com/apache/nifi/pull/5420#issuecomment-930143335
@gresockj Regarding the property type, it looks like the maximum read size should be an `int` since it corresponds to the protocol operation responsible for reading the size of received bytes.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] gresockj commented on pull request #5420: NIFI-9207 - limit read size, Distributed Cache Servers
Posted by GitBox <gi...@apache.org>.
gresockj commented on pull request #5420:
URL: https://github.com/apache/nifi/pull/5420#issuecomment-930380448
> Thanks making the changes @greyp9, this looks good!
>
> Do you have any additional feedback @gresockj?
Yes, one last question -- since the max read size should be an int, should we add additional validation to the Data Size Validator to ensure the value entered fits in the int range?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] asfgit closed pull request #5420: NIFI-9207 - limit read size, Distributed Cache Servers
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #5420:
URL: https://github.com/apache/nifi/pull/5420
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on pull request #5420: NIFI-9207 - limit read size, Distributed Cache Servers
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on pull request #5420:
URL: https://github.com/apache/nifi/pull/5420#issuecomment-930143335
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] gresockj commented on pull request #5420: NIFI-9207 - limit read size, Distributed Cache Servers
Posted by GitBox <gi...@apache.org>.
gresockj commented on pull request #5420:
URL: https://github.com/apache/nifi/pull/5420#issuecomment-930380448
> Thanks making the changes @greyp9, this looks good!
>
> Do you have any additional feedback @gresockj?
Yes, one last question -- since the max read size should be an int, should we add additional validation to the Data Size Validator to ensure the value entered fits in the int range?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on pull request #5420: NIFI-9207 - limit read size, Distributed Cache Servers
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on pull request #5420:
URL: https://github.com/apache/nifi/pull/5420#issuecomment-930396530
> > Thanks making the changes @greyp9, this looks good!
> > Do you have any additional feedback @gresockj?
>
> Yes, one last question -- since the max read size should be an int, should we add additional validation to the Data Size Validator to ensure the value entered fits in the int range?
That's a good question. The `asDataSize()` method returns a `Double`, but it looks the is a variety of usage currently, including both `longValue()` and `intValue()`. It seems like addressing that issue might be better in a separate issue focusing on `intValue()` usage.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] greyp9 commented on a change in pull request #5420: NIFI-9207 - limit read size, Distributed Cache Servers
Posted by GitBox <gi...@apache.org>.
greyp9 commented on a change in pull request #5420:
URL: https://github.com/apache/nifi/pull/5420#discussion_r718551880
##########
File path: nifi-nar-bundles/nifi-standard-services/nifi-distributed-cache-services-bundle/nifi-distributed-cache-server/src/main/java/org/apache/nifi/distributed/cache/server/DistributedCacheServer.java
##########
@@ -68,6 +68,13 @@
.required(false)
.addValidator(StandardValidators.createDirectoryExistsValidator(true, true))
.build();
+ public static final PropertyDescriptor MAX_READ_SIZE = new PropertyDescriptor.Builder()
+ .name("Maximum Read Size")
Review comment:
Cool; now I understand the PR template instructions. TIL!
##########
File path: nifi-nar-bundles/nifi-standard-services/nifi-distributed-cache-services-bundle/nifi-distributed-cache-server/src/main/java/org/apache/nifi/distributed/cache/server/map/MapCacheServer.java
##########
@@ -251,10 +251,17 @@ protected void finalize() throws Throwable {
}
private byte[] readValue(final DataInputStream dis) throws IOException {
- final int numBytes = dis.readInt();
+ final int numBytes = validateInt(dis.readInt(), getMaxReadSize(), "readValue():");
final byte[] buffer = new byte[numBytes];
dis.readFully(buffer);
return buffer;
}
+ private int validateInt(final int value, final int max, final String identifier) throws IOException {
+ if (value <= max) {
+ return value;
+ } else {
+ throw new IOException(new IllegalArgumentException(identifier + value));
+ }
+ }
Review comment:
I was thinking the same thing :)
##########
File path: nifi-nar-bundles/nifi-standard-services/nifi-distributed-cache-services-bundle/nifi-distributed-cache-client-service/src/main/java/org/apache/nifi/distributed/cache/client/CacheClientRequestHandler.java
##########
@@ -64,6 +64,18 @@ public void channelReadComplete(ChannelHandlerContext ctx) throws IOException {
}
}
+ @Override
+ public void channelUnregistered(ChannelHandlerContext ctx) {
Review comment:
Thanks. I'm used to this check occurring in the Maven build. I dug into my IDE settings and found the place to flag this.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] greyp9 commented on a change in pull request #5420: NIFI-9207 - limit read size, Distributed Cache Servers
Posted by GitBox <gi...@apache.org>.
greyp9 commented on a change in pull request #5420:
URL: https://github.com/apache/nifi/pull/5420#discussion_r718601402
##########
File path: nifi-nar-bundles/nifi-standard-services/nifi-distributed-cache-services-bundle/nifi-distributed-cache-client-service/src/main/java/org/apache/nifi/distributed/cache/client/CacheClientRequestHandler.java
##########
@@ -64,6 +64,18 @@ public void channelReadComplete(ChannelHandlerContext ctx) throws IOException {
}
}
+ @Override
+ public void channelUnregistered(ChannelHandlerContext ctx) {
Review comment:
Thanks. I'm used to this check occurring in the Maven build. I dug into my IDE settings and found the place to flag this.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] greyp9 commented on a change in pull request #5420: NIFI-9207 - limit read size, Distributed Cache Servers
Posted by GitBox <gi...@apache.org>.
greyp9 commented on a change in pull request #5420:
URL: https://github.com/apache/nifi/pull/5420#discussion_r718567092
##########
File path: nifi-nar-bundles/nifi-standard-services/nifi-distributed-cache-services-bundle/nifi-distributed-cache-server/src/main/java/org/apache/nifi/distributed/cache/server/map/MapCacheServer.java
##########
@@ -251,10 +251,17 @@ protected void finalize() throws Throwable {
}
private byte[] readValue(final DataInputStream dis) throws IOException {
- final int numBytes = dis.readInt();
+ final int numBytes = validateInt(dis.readInt(), getMaxReadSize(), "readValue():");
final byte[] buffer = new byte[numBytes];
dis.readFully(buffer);
return buffer;
}
+ private int validateInt(final int value, final int max, final String identifier) throws IOException {
+ if (value <= max) {
+ return value;
+ } else {
+ throw new IOException(new IllegalArgumentException(identifier + value));
+ }
+ }
Review comment:
I was thinking the same thing :)
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] asfgit closed pull request #5420: NIFI-9207 - limit read size, Distributed Cache Servers
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #5420:
URL: https://github.com/apache/nifi/pull/5420
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] greyp9 commented on a change in pull request #5420: NIFI-9207 - limit read size, Distributed Cache Servers
Posted by GitBox <gi...@apache.org>.
greyp9 commented on a change in pull request #5420:
URL: https://github.com/apache/nifi/pull/5420#discussion_r718551880
##########
File path: nifi-nar-bundles/nifi-standard-services/nifi-distributed-cache-services-bundle/nifi-distributed-cache-server/src/main/java/org/apache/nifi/distributed/cache/server/DistributedCacheServer.java
##########
@@ -68,6 +68,13 @@
.required(false)
.addValidator(StandardValidators.createDirectoryExistsValidator(true, true))
.build();
+ public static final PropertyDescriptor MAX_READ_SIZE = new PropertyDescriptor.Builder()
+ .name("Maximum Read Size")
Review comment:
Cool; now I understand the PR template instructions. TIL!
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on a change in pull request #5420: NIFI-9207 - limit read size, Distributed Cache Servers
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #5420:
URL: https://github.com/apache/nifi/pull/5420#discussion_r718040282
##########
File path: nifi-nar-bundles/nifi-standard-services/nifi-distributed-cache-services-bundle/nifi-distributed-cache-client-service/src/main/java/org/apache/nifi/distributed/cache/client/CacheClientRequestHandler.java
##########
@@ -64,6 +64,18 @@ public void channelReadComplete(ChannelHandlerContext ctx) throws IOException {
}
}
+ @Override
+ public void channelUnregistered(ChannelHandlerContext ctx) {
+ if (!inboundAdapter.isComplete()) {
+ channelPromise.setFailure(new IOException("channelUnregistered during request - " + ctx.channel().toString()));
Review comment:
What do you think about changing the message it indicate incomplete processing status?
```suggestion
channelPromise.setFailure(new IOException("Channel unregistered before processing completed: " + ctx.channel().toString()));
```
##########
File path: nifi-nar-bundles/nifi-standard-services/nifi-distributed-cache-services-bundle/nifi-distributed-cache-client-service/src/main/java/org/apache/nifi/distributed/cache/client/CacheClientRequestHandler.java
##########
@@ -86,5 +98,8 @@ public void invoke(final Channel channel, final OutboundAdapter outboundAdapter,
channel.writeAndFlush(Unpooled.wrappedBuffer(outboundAdapter.toBytes()));
channelPromise.awaitUninterruptibly();
this.inboundAdapter = new NullInboundAdapter();
+ if (channelPromise.cause() != null) {
+ throw new IOException(channelPromise.cause());
Review comment:
Recommend include a message with the exception:
```suggestion
throw new IOException("Request invocation failed", channelPromise.cause());
```
##########
File path: nifi-nar-bundles/nifi-standard-services/nifi-distributed-cache-services-bundle/nifi-distributed-cache-server/src/main/java/org/apache/nifi/distributed/cache/server/SetCacheServer.java
##########
@@ -102,4 +100,18 @@ protected void finalize() throws Throwable {
}
}
+ private byte[] readValue(final DataInputStream dis) throws IOException {
+ final int numBytes = validateInt(dis.readInt(), getMaxReadSize(), "readValue():");
+ final byte[] buffer = new byte[numBytes];
+ dis.readFully(buffer);
+ return buffer;
+ }
+
+ private int validateInt(final int value, final int max, final String identifier) throws IOException {
+ if (value <= max) {
+ return value;
+ } else {
+ throw new IOException(new IllegalArgumentException(identifier + value));
+ }
+ }
Review comment:
As mentioned above, recommend removing the `identifier` parameter and adjusting the exception message. The wrapping `IOException` also seems unnecessary.
```suggestion
private int validateSize(final int size) {
if (size <= getMaxReadSize()) {
return size;
} else {
throw new IllegalStateException(String.format("Size [%d] exceeds maximum configured read [%d]", size, getMaxReadSize()));
}
}
```
##########
File path: nifi-nar-bundles/nifi-standard-services/nifi-distributed-cache-services-bundle/nifi-distributed-cache-server/src/main/java/org/apache/nifi/distributed/cache/server/DistributedCacheServer.java
##########
@@ -68,6 +68,13 @@
.required(false)
.addValidator(StandardValidators.createDirectoryExistsValidator(true, true))
.build();
+ public static final PropertyDescriptor MAX_READ_SIZE = new PropertyDescriptor.Builder()
+ .name("Maximum Read Size")
Review comment:
A `displayName` should also be included:
```suggestion
.name("maximum-read-size")
.displayName("Maximum Read Size")
```
##########
File path: nifi-nar-bundles/nifi-standard-services/nifi-distributed-cache-services-bundle/nifi-distributed-cache-server/src/main/java/org/apache/nifi/distributed/cache/server/DistributedCacheServer.java
##########
@@ -68,6 +68,13 @@
.required(false)
.addValidator(StandardValidators.createDirectoryExistsValidator(true, true))
.build();
+ public static final PropertyDescriptor MAX_READ_SIZE = new PropertyDescriptor.Builder()
+ .name("Maximum Read Size")
+ .description("The maximum number of network bytes to read for a single cache item")
+ .required(false)
+ .addValidator(StandardValidators.POSITIVE_INTEGER_VALIDATOR)
+ .defaultValue("1000000")
Review comment:
Instead of using a positive integer, the `DATA_SIZE_VALIDATOR` supports standard formatting for data sizes:
```suggestion
.addValidator(StandardValidators.DATA_SIZE_VALIDATOR)
.defaultValue("1 MB")
```
##########
File path: nifi-nar-bundles/nifi-standard-services/nifi-distributed-cache-services-bundle/nifi-distributed-cache-server/src/main/java/org/apache/nifi/distributed/cache/server/SetCacheServer.java
##########
@@ -102,4 +100,18 @@ protected void finalize() throws Throwable {
}
}
+ private byte[] readValue(final DataInputStream dis) throws IOException {
+ final int numBytes = validateInt(dis.readInt(), getMaxReadSize(), "readValue():");
Review comment:
The `readValue():` parameter appears to be the only value passed to `validateInt()`, so it seems better to remove this argument.
```suggestion
final int numBytes = validateInt(dis.readInt());
```
##########
File path: nifi-nar-bundles/nifi-standard-services/nifi-distributed-cache-services-bundle/nifi-distributed-cache-server/src/main/java/org/apache/nifi/distributed/cache/server/map/MapCacheServer.java
##########
@@ -251,10 +251,17 @@ protected void finalize() throws Throwable {
}
private byte[] readValue(final DataInputStream dis) throws IOException {
- final int numBytes = dis.readInt();
+ final int numBytes = validateInt(dis.readInt(), getMaxReadSize(), "readValue():");
final byte[] buffer = new byte[numBytes];
dis.readFully(buffer);
return buffer;
}
+ private int validateInt(final int value, final int max, final String identifier) throws IOException {
+ if (value <= max) {
+ return value;
+ } else {
+ throw new IOException(new IllegalArgumentException(identifier + value));
+ }
+ }
Review comment:
Could this method be promoted to the superclass and shared in both `MapCacheServer` and `SetCacheServer`?
##########
File path: nifi-nar-bundles/nifi-standard-services/nifi-distributed-cache-services-bundle/nifi-distributed-cache-server/src/main/java/org/apache/nifi/distributed/cache/server/DistributedSetCacheServer.java
##########
@@ -35,6 +35,7 @@ protected CacheServer createCacheServer(final ConfigurationContext context) {
final SSLContextService sslContextService = context.getProperty(SSL_CONTEXT_SERVICE).asControllerService(SSLContextService.class);
final int maxSize = context.getProperty(MAX_CACHE_ENTRIES).asInteger();
final String evictionPolicyName = context.getProperty(EVICTION_POLICY).getValue();
+ final int maxReadSize = context.getProperty(MAX_READ_SIZE).asInteger();
Review comment:
Following the suggestion to use a Data Size Validator, this should be adjusted:
```suggestion
final int maxReadSize = context.getProperty(MAX_READ_SIZE).asDataSize(DataUnit.B).intValue();
```
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] gresockj commented on a change in pull request #5420: NIFI-9207 - limit read size, Distributed Cache Servers
Posted by GitBox <gi...@apache.org>.
gresockj commented on a change in pull request #5420:
URL: https://github.com/apache/nifi/pull/5420#discussion_r718375440
##########
File path: nifi-nar-bundles/nifi-standard-services/nifi-distributed-cache-services-bundle/nifi-distributed-cache-client-service/src/main/java/org/apache/nifi/distributed/cache/client/CacheClientRequestHandler.java
##########
@@ -64,6 +64,18 @@ public void channelReadComplete(ChannelHandlerContext ctx) throws IOException {
}
}
+ @Override
+ public void channelUnregistered(ChannelHandlerContext ctx) {
Review comment:
Consider `final` argument here
##########
File path: nifi-nar-bundles/nifi-standard-services/nifi-distributed-cache-services-bundle/nifi-distributed-cache-server/src/main/java/org/apache/nifi/distributed/cache/server/map/DistributedMapCacheServer.java
##########
@@ -67,14 +68,14 @@ protected CacheServer createCacheServer(final ConfigurationContext context) {
try {
final File persistenceDir = persistencePath == null ? null : new File(persistencePath);
- return createMapCacheServer(port, maxSize, sslContext, evictionPolicy, persistenceDir);
+ return createMapCacheServer(port, maxSize, sslContext, evictionPolicy, persistenceDir, maxReadSize);
} catch (final Exception e) {
throw new RuntimeException(e);
}
}
- protected MapCacheServer createMapCacheServer(int port, int maxSize, SSLContext sslContext, EvictionPolicy evictionPolicy, File persistenceDir) throws IOException {
- return new MapCacheServer(getIdentifier(), sslContext, port, maxSize, evictionPolicy, persistenceDir);
+ protected MapCacheServer createMapCacheServer(int port, int maxSize, SSLContext sslContext, EvictionPolicy evictionPolicy, File persistenceDir, int maxReadSize) throws IOException {
Review comment:
Recommend `final` arguments
##########
File path: nifi-nar-bundles/nifi-standard-services/nifi-distributed-cache-services-bundle/nifi-distributed-cache-server/src/main/java/org/apache/nifi/distributed/cache/server/DistributedSetCacheServer.java
##########
@@ -35,6 +35,7 @@ protected CacheServer createCacheServer(final ConfigurationContext context) {
final SSLContextService sslContextService = context.getProperty(SSL_CONTEXT_SERVICE).asControllerService(SSLContextService.class);
final int maxSize = context.getProperty(MAX_CACHE_ENTRIES).asInteger();
final String evictionPolicyName = context.getProperty(EVICTION_POLICY).getValue();
+ final int maxReadSize = context.getProperty(MAX_READ_SIZE).asInteger();
Review comment:
If we go with the Data Size Validator, should `maxReadSize` be a `long`? Anything over ~2.14 GB is over `Integer.MAX_SIZE`. Or if we want to limit the max data size, perhaps additional custom validation is needed.
##########
File path: nifi-nar-bundles/nifi-standard-services/nifi-distributed-cache-services-bundle/nifi-distributed-cache-client-service/src/main/java/org/apache/nifi/distributed/cache/client/CacheClientRequestHandler.java
##########
@@ -64,6 +64,18 @@ public void channelReadComplete(ChannelHandlerContext ctx) throws IOException {
}
}
+ @Override
+ public void channelUnregistered(ChannelHandlerContext ctx) {
+ if (!inboundAdapter.isComplete()) {
+ channelPromise.setFailure(new IOException("channelUnregistered during request - " + ctx.channel().toString()));
+ }
+ }
+
+ @Override
+ public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) {
Review comment:
Consider `final` arguments
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on a change in pull request #5420: NIFI-9207 - limit read size, Distributed Cache Servers
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #5420:
URL: https://github.com/apache/nifi/pull/5420#discussion_r718040282
##########
File path: nifi-nar-bundles/nifi-standard-services/nifi-distributed-cache-services-bundle/nifi-distributed-cache-client-service/src/main/java/org/apache/nifi/distributed/cache/client/CacheClientRequestHandler.java
##########
@@ -64,6 +64,18 @@ public void channelReadComplete(ChannelHandlerContext ctx) throws IOException {
}
}
+ @Override
+ public void channelUnregistered(ChannelHandlerContext ctx) {
+ if (!inboundAdapter.isComplete()) {
+ channelPromise.setFailure(new IOException("channelUnregistered during request - " + ctx.channel().toString()));
Review comment:
What do you think about changing the message it indicate incomplete processing status?
```suggestion
channelPromise.setFailure(new IOException("Channel unregistered before processing completed: " + ctx.channel().toString()));
```
##########
File path: nifi-nar-bundles/nifi-standard-services/nifi-distributed-cache-services-bundle/nifi-distributed-cache-client-service/src/main/java/org/apache/nifi/distributed/cache/client/CacheClientRequestHandler.java
##########
@@ -86,5 +98,8 @@ public void invoke(final Channel channel, final OutboundAdapter outboundAdapter,
channel.writeAndFlush(Unpooled.wrappedBuffer(outboundAdapter.toBytes()));
channelPromise.awaitUninterruptibly();
this.inboundAdapter = new NullInboundAdapter();
+ if (channelPromise.cause() != null) {
+ throw new IOException(channelPromise.cause());
Review comment:
Recommend include a message with the exception:
```suggestion
throw new IOException("Request invocation failed", channelPromise.cause());
```
##########
File path: nifi-nar-bundles/nifi-standard-services/nifi-distributed-cache-services-bundle/nifi-distributed-cache-server/src/main/java/org/apache/nifi/distributed/cache/server/SetCacheServer.java
##########
@@ -102,4 +100,18 @@ protected void finalize() throws Throwable {
}
}
+ private byte[] readValue(final DataInputStream dis) throws IOException {
+ final int numBytes = validateInt(dis.readInt(), getMaxReadSize(), "readValue():");
+ final byte[] buffer = new byte[numBytes];
+ dis.readFully(buffer);
+ return buffer;
+ }
+
+ private int validateInt(final int value, final int max, final String identifier) throws IOException {
+ if (value <= max) {
+ return value;
+ } else {
+ throw new IOException(new IllegalArgumentException(identifier + value));
+ }
+ }
Review comment:
As mentioned above, recommend removing the `identifier` parameter and adjusting the exception message. The wrapping `IOException` also seems unnecessary.
```suggestion
private int validateSize(final int size) {
if (size <= getMaxReadSize()) {
return size;
} else {
throw new IllegalStateException(String.format("Size [%d] exceeds maximum configured read [%d]", size, getMaxReadSize()));
}
}
```
##########
File path: nifi-nar-bundles/nifi-standard-services/nifi-distributed-cache-services-bundle/nifi-distributed-cache-server/src/main/java/org/apache/nifi/distributed/cache/server/DistributedCacheServer.java
##########
@@ -68,6 +68,13 @@
.required(false)
.addValidator(StandardValidators.createDirectoryExistsValidator(true, true))
.build();
+ public static final PropertyDescriptor MAX_READ_SIZE = new PropertyDescriptor.Builder()
+ .name("Maximum Read Size")
Review comment:
A `displayName` should also be included:
```suggestion
.name("maximum-read-size")
.displayName("Maximum Read Size")
```
##########
File path: nifi-nar-bundles/nifi-standard-services/nifi-distributed-cache-services-bundle/nifi-distributed-cache-server/src/main/java/org/apache/nifi/distributed/cache/server/DistributedCacheServer.java
##########
@@ -68,6 +68,13 @@
.required(false)
.addValidator(StandardValidators.createDirectoryExistsValidator(true, true))
.build();
+ public static final PropertyDescriptor MAX_READ_SIZE = new PropertyDescriptor.Builder()
+ .name("Maximum Read Size")
+ .description("The maximum number of network bytes to read for a single cache item")
+ .required(false)
+ .addValidator(StandardValidators.POSITIVE_INTEGER_VALIDATOR)
+ .defaultValue("1000000")
Review comment:
Instead of using a positive integer, the `DATA_SIZE_VALIDATOR` supports standard formatting for data sizes:
```suggestion
.addValidator(StandardValidators.DATA_SIZE_VALIDATOR)
.defaultValue("1 MB")
```
##########
File path: nifi-nar-bundles/nifi-standard-services/nifi-distributed-cache-services-bundle/nifi-distributed-cache-server/src/main/java/org/apache/nifi/distributed/cache/server/SetCacheServer.java
##########
@@ -102,4 +100,18 @@ protected void finalize() throws Throwable {
}
}
+ private byte[] readValue(final DataInputStream dis) throws IOException {
+ final int numBytes = validateInt(dis.readInt(), getMaxReadSize(), "readValue():");
Review comment:
The `readValue():` parameter appears to be the only value passed to `validateInt()`, so it seems better to remove this argument.
```suggestion
final int numBytes = validateInt(dis.readInt());
```
##########
File path: nifi-nar-bundles/nifi-standard-services/nifi-distributed-cache-services-bundle/nifi-distributed-cache-server/src/main/java/org/apache/nifi/distributed/cache/server/map/MapCacheServer.java
##########
@@ -251,10 +251,17 @@ protected void finalize() throws Throwable {
}
private byte[] readValue(final DataInputStream dis) throws IOException {
- final int numBytes = dis.readInt();
+ final int numBytes = validateInt(dis.readInt(), getMaxReadSize(), "readValue():");
final byte[] buffer = new byte[numBytes];
dis.readFully(buffer);
return buffer;
}
+ private int validateInt(final int value, final int max, final String identifier) throws IOException {
+ if (value <= max) {
+ return value;
+ } else {
+ throw new IOException(new IllegalArgumentException(identifier + value));
+ }
+ }
Review comment:
Could this method be promoted to the superclass and shared in both `MapCacheServer` and `SetCacheServer`?
##########
File path: nifi-nar-bundles/nifi-standard-services/nifi-distributed-cache-services-bundle/nifi-distributed-cache-server/src/main/java/org/apache/nifi/distributed/cache/server/DistributedSetCacheServer.java
##########
@@ -35,6 +35,7 @@ protected CacheServer createCacheServer(final ConfigurationContext context) {
final SSLContextService sslContextService = context.getProperty(SSL_CONTEXT_SERVICE).asControllerService(SSLContextService.class);
final int maxSize = context.getProperty(MAX_CACHE_ENTRIES).asInteger();
final String evictionPolicyName = context.getProperty(EVICTION_POLICY).getValue();
+ final int maxReadSize = context.getProperty(MAX_READ_SIZE).asInteger();
Review comment:
Following the suggestion to use a Data Size Validator, this should be adjusted:
```suggestion
final int maxReadSize = context.getProperty(MAX_READ_SIZE).asDataSize(DataUnit.B).intValue();
```
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org