exceptionfactory commented on code in PR #6996:
URL: https://github.com/apache/nifi/pull/6996#discussion_r1149511090
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/controller/repository/io/ContentClaimInputStream.java:
##########
@@ -81,14 +81,13 @@ public long getCurrentOffset() {
@Override
public int read() throws IOException {
- int value = -1;
+ int value;
Review Comment:
With this change, it looks like `value` can be marked `final`:
```suggestion
final int value;
```
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/controller/repository/io/ContentClaimInputStream.java:
##########
@@ -117,11 +115,10 @@ public int read(final byte[] b) throws IOException {
@Override
public int read(final byte[] b, final int off, final int len) throws
IOException {
- int count = -1;
+ int count;
Review Comment:
```suggestion
final int count;
```
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/controller/repository/io/ContentClaimInputStream.java:
##########
@@ -158,26 +155,47 @@ public boolean markSupported() {
return true;
}
+ /**
+ * Marks the current position. Can be returned to with {@code reset()}.
+ *
+ * @see ContentClaimInputStream#reset()
+ * @param readlimit hint on how much data should be buffered.
+ */
@Override
public void mark(final int readlimit) {
markOffset = currentOffset;
markReadLimit = readlimit;
- if (bufferedIn != null) {
- bufferedIn.mark(readlimit);
+ if (bufferedIn == null) {
+ try {
+ bufferedIn = new BufferedInputStream(getDelegate());
+ } catch (IOException ex) {
+ throw new RuntimeException("Failed to read content claim!",
ex);
Review Comment:
Instead of throwing a `RuntimeException`, it looks like an
`UncheckedIOException` would be more appropriate.
As a general rule, exclamation points should be avoided in error messages,
so see the following suggested wording:
```suggestion
} catch (IOException e) {
throw new UncheckedIOException("Failed to read repository
Content Claim", e);
```
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/controller/repository/io/ContentClaimInputStream.java:
##########
@@ -200,6 +218,10 @@ public void reset() throws IOException {
@Override
public void close() throws IOException {
+ if (bufferedIn != null) {
Review Comment:
Minor spacing issue:
```suggestion
if (bufferedIn != null) {
```
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/controller/repository/io/ContentClaimInputStream.java:
##########
@@ -215,15 +237,6 @@ private void formDelegate() throws IOException {
delegate = new
PerformanceTrackingInputStream(contentRepository.read(contentClaim),
performanceTracker);
StreamUtils.skip(delegate, claimOffset);
currentOffset = claimOffset;
-
- if (markReadLimit > 0) {
- final int limitLeft = (int) (markReadLimit - currentOffset);
- if (limitLeft > 0) {
- final InputStream limitedIn = new
LimitedInputStream(delegate, limitLeft);
- bufferedIn = new BufferedInputStream(limitedIn);
- bufferedIn.mark(limitLeft);
- }
- }
Review Comment:
For clarification, is this removed so that the buffer is only created when
calling `mark()`?
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/controller/repository/io/ContentClaimInputStream.java:
##########
@@ -158,26 +155,47 @@ public boolean markSupported() {
return true;
}
+ /**
+ * Marks the current position. Can be returned to with {@code reset()}.
+ *
+ * @see ContentClaimInputStream#reset()
+ * @param readlimit hint on how much data should be buffered.
+ */
@Override
public void mark(final int readlimit) {
markOffset = currentOffset;
markReadLimit = readlimit;
- if (bufferedIn != null) {
- bufferedIn.mark(readlimit);
+ if (bufferedIn == null) {
+ try {
+ bufferedIn = new BufferedInputStream(getDelegate());
+ } catch (IOException ex) {
+ throw new RuntimeException("Failed to read content claim!",
ex);
+ }
}
+
+ bufferedIn.mark(readlimit);
}
+ /**
+ * Resets to the last marked position.
+ *
+ * @see ContentClaimInputStream#mark(int)
+ * @throws IOException only if no mark point has been set.
Review Comment:
The `IOException` can also be thrown from other operations such as resetting
or closing the buffer or delegate stream, so recommend the following wording:
```suggestion
* @throws IOException Thrown when a mark position is not set or on
other stream handling failures
```
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/test/java/org/apache/nifi/controller/repository/io/TestContentClaimInputStream.java:
##########
@@ -183,11 +184,88 @@ public void testRereadWithOffset() throws IOException {
in.reset();
}
- Mockito.verify(repo, Mockito.times(invocations +
1)).read(contentClaim); // Will call reset() 'invocations' times plus the
initial read
+ Mockito.verify(repo, Mockito.times( 1)).read(contentClaim);
Review Comment:
```suggestion
Mockito.verify(repo).read(contentClaim);
```
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/test/java/org/apache/nifi/controller/repository/io/TestContentClaimInputStream.java:
##########
@@ -152,7 +153,7 @@ public void testMultipleResetCallsAfterMark() throws
IOException {
in.reset();
}
- Mockito.verify(repo, Mockito.times(invocations +
1)).read(contentClaim); // Will call reset() 'invocations' times plus the
initial read
+ Mockito.verify(repo, Mockito.times( 1)).read(contentClaim);
Review Comment:
```suggestion
Mockito.verify(repo).read(contentClaim);
```
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/test/java/org/apache/nifi/controller/repository/io/TestContentClaimInputStream.java:
##########
@@ -121,7 +122,7 @@ public void testRereadEntireClaim() throws IOException {
in.reset();
}
- Mockito.verify(repo, Mockito.times(invocations +
1)).read(contentClaim); // Will call reset() 'invocations' times plus the
initial read
+ Mockito.verify(repo, Mockito.times( 1)).read(contentClaim);
Review Comment:
The `times()` call is not necessary since the default behavior of `verify()`
expects 1 invocation.
```suggestion
Mockito.verify(repo).read(contentClaim);
```
--
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]
