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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
