[GitHub] [nifi] ijokarumawak commented on a change in pull request #3647: NIFI-6530 - HTTP SiteToSite server returns 201 in case no data is ava…
ijokarumawak commented on a change in pull request #3647: NIFI-6530 - HTTP
SiteToSite server returns 201 in case no data is ava…
URL: https://github.com/apache/nifi/pull/3647#discussion_r321067035
##
File path:
nifi-commons/nifi-site-to-site-client/src/test/java/org/apache/nifi/remote/client/TestPeerSelector.java
##
@@ -212,44 +213,48 @@ public void testFetchRemotePeerStatuses() throws
IOException {
throw new IOException("Connection refused. " +
peerFetchStatusesFrom + " is not running.");
}).when(peerStatusProvider).fetchRemotePeerStatuses(any(PeerDescription.class));
+
+ArrayList peers;
+
// 1st attempt. It uses the bootstrap node.
peerSelector.refreshPeers();
-PeerStatus peerStatus =
peerSelector.getNextPeerStatus(TransferDirection.RECEIVE);
-assertNotNull(peerStatus);
+peers = peerSelector.getPeerStatuses(TransferDirection.RECEIVE);
+assert(!peers.isEmpty());
// Proceed time so that peer selector refresh statuses.
peerStatuses.remove(bootstrapNodeStatus);
systemTime.offset += TimeUnit.MILLISECONDS.convert(1,
TimeUnit.MINUTES) + 1;
// 2nd attempt.
peerSelector.refreshPeers();
-peerStatus = peerSelector.getNextPeerStatus(TransferDirection.RECEIVE);
-assertNotNull(peerStatus);
-assertEquals("Node2 should be returned since node 2 is the only
available node.", node2, peerStatus.getPeerDescription());
+peers = peerSelector.getPeerStatuses(TransferDirection.RECEIVE);
+assert(!peers.isEmpty());
+assertEquals("Node2 should be returned since node 2 is the only
available node.", node2, peers.get(0).getPeerDescription());
// Proceed time so that peer selector refresh statuses.
systemTime.offset += TimeUnit.MILLISECONDS.convert(1,
TimeUnit.MINUTES) + 1;
// 3rd attempt.
peerSelector.refreshPeers();
-peerStatus = peerSelector.getNextPeerStatus(TransferDirection.RECEIVE);
-assertNotNull(peerStatus);
-assertEquals("Node2 should be returned since node 2 is the only
available node.", node2, peerStatus.getPeerDescription());
+peers = peerSelector.getPeerStatuses(TransferDirection.RECEIVE);
+assert(!peers.isEmpty());
+assertEquals("Node2 should be returned since node 2 is the only
available node.", node2, peers.get(0).getPeerDescription());
// Remove node2 to simulate that it goes down. There's no available
node at this point.
peerStatuses.remove(node2Status);
systemTime.offset += TimeUnit.MILLISECONDS.convert(1,
TimeUnit.MINUTES) + 1;
peerSelector.refreshPeers();
-peerStatus = peerSelector.getNextPeerStatus(TransferDirection.RECEIVE);
-assertNull("PeerSelector should return null as next peer status, since
there's no available peer", peerStatus);
+peers = peerSelector.getPeerStatuses(TransferDirection.RECEIVE);
+assertTrue("PeerSelector should return null as next peer status, since
there's no available peer", peers.isEmpty());
Review comment:
Knit-picking...
"PeerSelector should return null as next peer status"
should be
"PeerSelector should return an empty list as next peer statuses"
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] ijokarumawak commented on a change in pull request #3647: NIFI-6530 - HTTP SiteToSite server returns 201 in case no data is ava…
ijokarumawak commented on a change in pull request #3647: NIFI-6530 - HTTP
SiteToSite server returns 201 in case no data is ava…
URL: https://github.com/apache/nifi/pull/3647#discussion_r321068442
##
File path:
nifi-stateless/nifi-stateless-core/src/main/java/org/apache/nifi/stateless/core/StatelessRemoteOutputPort.java
##
@@ -139,7 +140,11 @@ public boolean runRecursive(final Queue
queue) {
transaction.confirm();
transaction.complete();
-} catch (final Exception e) {
+} catch (final NoValidPeerException e) {
+getLogger().error("Unable to create a transaction for Remote
Process Group {} to pull from port {}", new Object[]{url, name});
+return false;
+}
+catch (final Exception e) {
Review comment:
Please combine this `catch` line with the previous line.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] ijokarumawak commented on a change in pull request #3647: NIFI-6530 - HTTP SiteToSite server returns 201 in case no data is ava…
ijokarumawak commented on a change in pull request #3647: NIFI-6530 - HTTP
SiteToSite server returns 201 in case no data is ava…
URL: https://github.com/apache/nifi/pull/3647#discussion_r321076934
##
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/api/DataTransferResource.java
##
@@ -205,9 +208,20 @@ public Response createPortTransaction(
final int transportProtocolVersion =
validationResult.transportProtocolVersion;
try {
-// Execute handshake.
-initiateServerProtocol(req, peer, transportProtocolVersion);
+HttpFlowFileServerProtocol serverProtocol =
initiateServerProtocol(req, peer, transportProtocolVersion);
+
+int protocolVersion =
Integer.parseUnsignedInt(req.getHeader(PROTOCOL_VERSION));
+if ((protocolVersion >= 2) && PORT_TYPE_OUTPUT.equals(portType)) {
+List connectionList =
serverProtocol.getPort().getIncomingConnections();
+if (connectionList.stream().allMatch(c ->
c.getFlowFileQueue().isEmpty())) {
+// Transaction could be created, but there is nothing to
transfer. Just return 200.
+logger.debug("Output port has no flowfiles to transfer,
returning 200");
+return
noCache(Response.status(Response.Status.NO_CONTENT)).type(MediaType.TEXT_PLAIN).entity("No
flowfiles available").build();
Review comment:
A transaction is already created at this point, by the preceding following
line:
```
final String transactionId = transactionManager.createTransaction();
```
The created transaction will be removed by the back-ground maintenance
thread, but we should remove it from here immediately by calling
`transactionManager.cancelTransaction(transactionId)`.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] ijokarumawak commented on a change in pull request #3647: NIFI-6530 - HTTP SiteToSite server returns 201 in case no data is ava…
ijokarumawak commented on a change in pull request #3647: NIFI-6530 - HTTP
SiteToSite server returns 201 in case no data is ava…
URL: https://github.com/apache/nifi/pull/3647#discussion_r321066722
##
File path:
nifi-commons/nifi-site-to-site-client/src/main/java/org/apache/nifi/remote/client/socket/SocketClient.java
##
@@ -125,15 +126,15 @@ public Transaction createTransaction(final
TransferDirection direction) throws I
}
final EndpointConnection connectionState =
pool.getEndpointConnection(direction, getConfig());
-if (connectionState == null) {
-return null;
-}
final Transaction transaction;
try {
transaction =
connectionState.getSocketClientProtocol().startTransaction(
connectionState.getPeer(), connectionState.getCodec(),
direction);
-} catch (final Throwable t) {
+} catch (final NoContentException e) {
+return null;
+}
+catch (final Throwable t) {
Review comment:
The previous closing curly bracket and this `catch` should be in the same
line.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] ijokarumawak commented on a change in pull request #3647: NIFI-6530 - HTTP SiteToSite server returns 201 in case no data is ava…
ijokarumawak commented on a change in pull request #3647: NIFI-6530 - HTTP
SiteToSite server returns 201 in case no data is ava…
URL: https://github.com/apache/nifi/pull/3647#discussion_r314555681
##
File path:
nifi-commons/nifi-site-to-site-client/src/main/java/org/apache/nifi/remote/client/http/HttpClient.java
##
@@ -168,6 +169,9 @@ public Transaction createTransaction(final
TransferDirection direction) throws H
try {
transactionUrl = apiClient.initiateTransaction(direction,
portId);
commSession.setUserDn(apiClient.getTrustedPeerDn());
+} catch (final NoContentException e) {
+logger.debug("Peer {} has no flowfiles to provide", peer);
+throw e;
Review comment:
I totally agree with you on the necessity of separating the two cases. How
about changing SiteToSiteClient.createTransaction semantics as follows?
- AS IS
- HandshakeException, PortNotRunningException, UnknownPortException in
irregular case
- Returns a Transaction to use for sending or receiving data, or `null`
if all nodes are penalized.
- TO BE (by this PR)
- HandshakeException, PortNotRunningException, UnknownPortException in
irregular case
- Or (new) NoPeerAvailableException when all peers are penalized.
Because this is an error case. We need to change
[SocketClient](https://github.com/apache/nifi/blob/master/nifi-commons/nifi-site-to-site-client/src/main/java/org/apache/nifi/remote/client/socket/SocketClient.java#L129),
too.
- Returns a Transaction to use for sending or receiving data, or `null`
if communication was successful but no transaction is needed, ex. no data to
transfer.
This allows callers to handle each cases correctly, and the API looks more
intuitive. How do you think?
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] ijokarumawak commented on a change in pull request #3647: NIFI-6530 - HTTP SiteToSite server returns 201 in case no data is ava…
ijokarumawak commented on a change in pull request #3647: NIFI-6530 - HTTP
SiteToSite server returns 201 in case no data is ava…
URL: https://github.com/apache/nifi/pull/3647#discussion_r314230906
##
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-site-to-site/src/main/java/org/apache/nifi/remote/StandardRemoteGroupPort.java
##
@@ -236,6 +237,11 @@ public void onTrigger(final ProcessContext context, final
ProcessSession session
final Transaction transaction;
try {
transaction = client.createTransaction(transferDirection);
+} catch (final NoContentException e) {
+context.yield();
+final String message = String.format("%s successfully connected to
%s, but it has no flowfiles to provide, yielding", this, url);
+logger.info(message);
Review comment:
But if we return `null` transaction, this catch block is no longer needed.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] ijokarumawak commented on a change in pull request #3647: NIFI-6530 - HTTP SiteToSite server returns 201 in case no data is ava…
ijokarumawak commented on a change in pull request #3647: NIFI-6530 - HTTP
SiteToSite server returns 201 in case no data is ava…
URL: https://github.com/apache/nifi/pull/3647#discussion_r314227662
##
File path:
nifi-commons/nifi-site-to-site-client/src/main/java/org/apache/nifi/remote/client/http/HttpClient.java
##
@@ -168,6 +169,9 @@ public Transaction createTransaction(final
TransferDirection direction) throws H
try {
transactionUrl = apiClient.initiateTransaction(direction,
portId);
commSession.setUserDn(apiClient.getTrustedPeerDn());
+} catch (final NoContentException e) {
+logger.debug("Peer {} has no flowfiles to provide", peer);
+throw e;
Review comment:
If the remote NiFi is a cluster and there are multiple NiFi nodes, even if
this client get NoContentException from node1, node2 may have queued data to
transfer. I think we should keep iterating next peer in this case, instead of
throwing the exception.
If no node has data, this method eventually returns `null` transaction. From
callers, it looks the same with the case where all nodes are penalized. I think
that is acceptable.
If we take this approach, we don't have to catch `NoContentException` at
callers such as StandardRemoteGroupPort.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] ijokarumawak commented on a change in pull request #3647: NIFI-6530 - HTTP SiteToSite server returns 201 in case no data is ava…
ijokarumawak commented on a change in pull request #3647: NIFI-6530 - HTTP SiteToSite server returns 201 in case no data is ava… URL: https://github.com/apache/nifi/pull/3647#discussion_r314229746 ## File path: nifi-commons/nifi-site-to-site-client/src/main/java/org/apache/nifi/remote/exception/NoContentException.java ## @@ -0,0 +1,40 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.nifi.remote.exception; + +import java.io.IOException; + +/** + * A ProtocolException occurs when unexpected data is received, for example an + * invalid Response Code. Review comment: This Java doc comment should be updated. 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [nifi] ijokarumawak commented on a change in pull request #3647: NIFI-6530 - HTTP SiteToSite server returns 201 in case no data is ava…
ijokarumawak commented on a change in pull request #3647: NIFI-6530 - HTTP
SiteToSite server returns 201 in case no data is ava…
URL: https://github.com/apache/nifi/pull/3647#discussion_r314222065
##
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-site-to-site/src/main/java/org/apache/nifi/remote/StandardRemoteGroupPort.java
##
@@ -236,6 +237,11 @@ public void onTrigger(final ProcessContext context, final
ProcessSession session
final Transaction transaction;
try {
transaction = client.createTransaction(transferDirection);
+} catch (final NoContentException e) {
+context.yield();
+final String message = String.format("%s successfully connected to
%s, but it has no flowfiles to provide, yielding", this, url);
+logger.info(message);
Review comment:
I'd lower log level to debug.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] ijokarumawak commented on a change in pull request #3647: NIFI-6530 - HTTP SiteToSite server returns 201 in case no data is ava…
ijokarumawak commented on a change in pull request #3647: NIFI-6530 - HTTP
SiteToSite server returns 201 in case no data is ava…
URL: https://github.com/apache/nifi/pull/3647#discussion_r313307594
##
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/api/DataTransferResource.java
##
@@ -205,9 +207,17 @@ public Response createPortTransaction(
final int transportProtocolVersion =
validationResult.transportProtocolVersion;
try {
-// Execute handshake.
-initiateServerProtocol(req, peer, transportProtocolVersion);
+HttpFlowFileServerProtocol serverProtocol =
initiateServerProtocol(req, peer, transportProtocolVersion);
+if (PORT_TYPE_OUTPUT.equals(portType)) {
+List connectionList =
serverProtocol.getPort().getIncomingConnections();
+if (connectionList.stream().allMatch(c ->
c.getFlowFileQueue().isEmpty())) {
+// Transaction could be created, but there is nothing to
transfer. Just return 200.
+logger.debug("Output port has no flowfiles to transfer,
returning 200");
Review comment:
Also, we don't want to penalize a peer at a client in this case, because if
we penalize, the peer will not be used for a certain period of time and that
will adds some latency for the next data transfer.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] ijokarumawak commented on a change in pull request #3647: NIFI-6530 - HTTP SiteToSite server returns 201 in case no data is ava…
ijokarumawak commented on a change in pull request #3647: NIFI-6530 - HTTP
SiteToSite server returns 201 in case no data is ava…
URL: https://github.com/apache/nifi/pull/3647#discussion_r313302446
##
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/api/DataTransferResource.java
##
@@ -205,9 +207,17 @@ public Response createPortTransaction(
final int transportProtocolVersion =
validationResult.transportProtocolVersion;
try {
-// Execute handshake.
-initiateServerProtocol(req, peer, transportProtocolVersion);
+HttpFlowFileServerProtocol serverProtocol =
initiateServerProtocol(req, peer, transportProtocolVersion);
+if (PORT_TYPE_OUTPUT.equals(portType)) {
+List connectionList =
serverProtocol.getPort().getIncomingConnections();
+if (connectionList.stream().allMatch(c ->
c.getFlowFileQueue().isEmpty())) {
+// Transaction could be created, but there is nothing to
transfer. Just return 200.
+logger.debug("Output port has no flowfiles to transfer,
returning 200");
Review comment:
I agree with the idea of using 204 in this case. Regardless of using 200 or
204, current Java HTTP S2S client will treat such response codes as unexpected
and throws an exception. Then users will see following warning message in
nifi-app.log:
```
2019-08-13 18:37:39,475 WARN [Timer-Driven Process Thread-9]
o.a.nifi.remote.client.http.HttpClient Penalizing a peer
Peer[url=http://HW13076.local:8080/nifi-api] due to java.io.IOException:
Unexpected response code: 200 errCode:Abort errMessage:No flowfiles available
2019-08-13 18:37:45,593 INFO [Timer-Driven Process Thread-9]
o.a.nifi.remote.client.http.HttpClient Couldn't find a valid peer to
communicate with.
```
In order to make this improvement happen while supporting old S2S clients, I
think we need to:
- Increase HTTP S2S protocol version from 1 to 2. Let's use 204 if we
increase protocol version. Please refer:
- 'Protocol version management' at this [wiki
page](https://cwiki.apache.org/confluence/display/NIFI/Support+HTTP%28S%29+as+a+transport+mechanism+for+Site-to-Site)
- You can find related code by searching usage of [this
constant](https://github.com/apache/nifi/blob/master/nifi-commons/nifi-site-to-site-client/src/main/java/org/apache/nifi/remote/protocol/http/HttpHeaders.java#L27)
- Return 204 only for clients using protocol version 2 or higher. Keep
acting the same for older clients.
- Update Java HTTP S2S client, too, to properly handle 204.
This requires more changes than I thought, but will provide ideal response
code (204) and backward compatibility. How do you think?
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] ijokarumawak commented on a change in pull request #3647: NIFI-6530 - HTTP SiteToSite server returns 201 in case no data is ava…
ijokarumawak commented on a change in pull request #3647: NIFI-6530 - HTTP
SiteToSite server returns 201 in case no data is ava…
URL: https://github.com/apache/nifi/pull/3647#discussion_r313302446
##
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/api/DataTransferResource.java
##
@@ -205,9 +207,17 @@ public Response createPortTransaction(
final int transportProtocolVersion =
validationResult.transportProtocolVersion;
try {
-// Execute handshake.
-initiateServerProtocol(req, peer, transportProtocolVersion);
+HttpFlowFileServerProtocol serverProtocol =
initiateServerProtocol(req, peer, transportProtocolVersion);
+if (PORT_TYPE_OUTPUT.equals(portType)) {
+List connectionList =
serverProtocol.getPort().getIncomingConnections();
+if (connectionList.stream().allMatch(c ->
c.getFlowFileQueue().isEmpty())) {
+// Transaction could be created, but there is nothing to
transfer. Just return 200.
+logger.debug("Output port has no flowfiles to transfer,
returning 200");
Review comment:
I agree with the idea of using 204 in this case. Regardless of using 200 or
204, current Java HTTP S2S client will treat such response codes as unexpected
and throws an exception. Then users will see following error message:
In order to make this improvement happen while supporting old S2S clients, I
think we need to:
- Update Java HTTP S2S client, too, to properly handle 204. Let's use 204
since we increase protocol version.
- Increase HTTP S2S protocol version from 1 to 2. Please refer:
- 'Protocol version management' at this [wiki
page](https://cwiki.apache.org/confluence/display/NIFI/Support+HTTP%28S%29+as+a+transport+mechanism+for+Site-to-Site)
- You can find related code by searching usage of [this
constant](https://github.com/apache/nifi/blob/master/nifi-commons/nifi-site-to-site-client/src/main/java/org/apache/nifi/remote/protocol/http/HttpHeaders.java#L27)
- Return 204 only for clients using protocol version 2 or higher. Keep
acting the same for older clients.
This requires more changes than I thought, but will provide ideal response
code (204) and backward compatibility. How do you think?
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:
us...@infra.apache.org
With regards,
Apache Git Services
