You are viewing a plain text version of this content. The canonical link for it is here.
Posted to common-issues@hadoop.apache.org by "steveloughran (via GitHub)" <gi...@apache.org> on 2023/02/07 19:28:38 UTC
[GitHub] [hadoop] steveloughran opened a new pull request, #5366: HDFS-16853. IPC shutdown failures.
steveloughran opened a new pull request, #5366:
URL: https://github.com/apache/hadoop/pull/5366
### Description of PR
Extension of #5162
Trying to address the problem
1. MUST NOT submit into blocking queue while closing
2. MUST NOT call queue.put() in synchronous block.
This design doesn't quite stop (2), though it should
detect and warn if the problem surfaces.
"Possible overlap in queue shutdown and request"
No new tests;
### How was this patch tested?
### For code changes:
- [X] Does the title or this PR starts with the corresponding JIRA issue id (e.g. 'HADOOP-17799. Your PR title ...')?
- [ ] Object storage: have the integration tests been executed and the endpoint declared according to the connector-specific documentation?
- [ ] 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`, `LICENSE-binary`, `NOTICE-binary` files?
--
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: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] hadoop-yetus commented on pull request #5366: HDFS-16853. IPC shutdown failures.
Posted by "hadoop-yetus (via GitHub)" <gi...@apache.org>.
hadoop-yetus commented on PR #5366:
URL: https://github.com/apache/hadoop/pull/5366#issuecomment-1423057647
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|:----:|----------:|--------:|:--------:|:-------:|
| +0 :ok: | reexec | 0m 49s | | Docker mode activated. |
|||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | | No case conflicting files found. |
| +0 :ok: | codespell | 0m 1s | | codespell was not available. |
| +0 :ok: | detsecrets | 0m 1s | | detect-secrets was not available. |
| +1 :green_heart: | @author | 0m 0s | | The patch does not contain any @author tags. |
| +1 :green_heart: | test4tests | 0m 0s | | The patch appears to include 1 new or modified test files. |
|||| _ trunk Compile Tests _ |
| +1 :green_heart: | mvninstall | 46m 9s | | trunk passed |
| +1 :green_heart: | compile | 25m 13s | | trunk passed with JDK Ubuntu-11.0.17+8-post-Ubuntu-1ubuntu220.04 |
| +1 :green_heart: | compile | 21m 46s | | trunk passed with JDK Private Build-1.8.0_352-8u352-ga-1~20.04-b08 |
| +1 :green_heart: | checkstyle | 1m 7s | | trunk passed |
| +1 :green_heart: | mvnsite | 1m 38s | | trunk passed |
| +1 :green_heart: | javadoc | 1m 7s | | trunk passed with JDK Ubuntu-11.0.17+8-post-Ubuntu-1ubuntu220.04 |
| +1 :green_heart: | javadoc | 0m 40s | | trunk passed with JDK Private Build-1.8.0_352-8u352-ga-1~20.04-b08 |
| +1 :green_heart: | spotbugs | 2m 38s | | trunk passed |
| +1 :green_heart: | shadedclient | 28m 5s | | branch has no errors when building and testing our client artifacts. |
|||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 1m 6s | | the patch passed |
| +1 :green_heart: | compile | 24m 36s | | the patch passed with JDK Ubuntu-11.0.17+8-post-Ubuntu-1ubuntu220.04 |
| +1 :green_heart: | javac | 24m 36s | | the patch passed |
| +1 :green_heart: | compile | 21m 28s | | the patch passed with JDK Private Build-1.8.0_352-8u352-ga-1~20.04-b08 |
| +1 :green_heart: | javac | 21m 28s | | the patch passed |
| +1 :green_heart: | blanks | 0m 0s | | The patch has no blanks issues. |
| -0 :warning: | checkstyle | 0m 59s | [/results-checkstyle-hadoop-common-project_hadoop-common.txt](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5366/2/artifact/out/results-checkstyle-hadoop-common-project_hadoop-common.txt) | hadoop-common-project/hadoop-common: The patch generated 1 new + 127 unchanged - 0 fixed = 128 total (was 127) |
| +1 :green_heart: | mvnsite | 1m 36s | | the patch passed |
| +1 :green_heart: | javadoc | 0m 59s | | the patch passed with JDK Ubuntu-11.0.17+8-post-Ubuntu-1ubuntu220.04 |
| +1 :green_heart: | javadoc | 0m 40s | | the patch passed with JDK Private Build-1.8.0_352-8u352-ga-1~20.04-b08 |
| +1 :green_heart: | spotbugs | 2m 41s | | the patch passed |
| +1 :green_heart: | shadedclient | 28m 25s | | patch has no errors when building and testing our client artifacts. |
|||| _ Other Tests _ |
| -1 :x: | unit | 18m 52s | [/patch-unit-hadoop-common-project_hadoop-common.txt](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5366/2/artifact/out/patch-unit-hadoop-common-project_hadoop-common.txt) | hadoop-common in the patch passed. |
| +1 :green_heart: | asflicense | 0m 52s | | The patch does not generate ASF License warnings. |
| | | 231m 1s | | |
| Reason | Tests |
|-------:|:------|
| Failed junit tests | hadoop.ipc.TestRPCWaitForProxy |
| | hadoop.ipc.TestSaslRPC |
| | hadoop.ipc.TestRPC |
| | hadoop.ipc.TestIPC |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.42 ServerAPI=1.42 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5366/2/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hadoop/pull/5366 |
| Optional Tests | dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell detsecrets |
| uname | Linux c021f31b2fff 4.15.0-200-generic #211-Ubuntu SMP Thu Nov 24 18:16:04 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/bin/hadoop.sh |
| git revision | trunk / e0bbcbf1cb745dc0f2958fb7cd6e331cd8b80222 |
| Default Java | Private Build-1.8.0_352-8u352-ga-1~20.04-b08 |
| Multi-JDK versions | /usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.17+8-post-Ubuntu-1ubuntu220.04 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_352-8u352-ga-1~20.04-b08 |
| Test Results | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5366/2/testReport/ |
| Max. process+thread count | 3165 (vs. ulimit of 5500) |
| modules | C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common |
| Console output | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5366/2/console |
| versions | git=2.25.1 maven=3.6.3 spotbugs=4.2.2 |
| Powered by | Apache Yetus 0.14.0 https://yetus.apache.org |
This message was automatically generated.
--
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: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] steveloughran commented on pull request #5366: HDFS-16853. IPC shutdown failures.
Posted by "steveloughran (via GitHub)" <gi...@apache.org>.
steveloughran commented on PR #5366:
URL: https://github.com/apache/hadoop/pull/5366#issuecomment-1455856950
owen actually fixed this properly
--
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: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] steveloughran commented on a diff in pull request #5366: HDFS-16853. IPC shutdown failures.
Posted by "steveloughran (via GitHub)" <gi...@apache.org>.
steveloughran commented on code in PR #5366:
URL: https://github.com/apache/hadoop/pull/5366#discussion_r1099988188
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/Client.java:
##########
@@ -1241,9 +1301,28 @@ private void receiveRpcResponse() {
markClosed(e);
}
}
-
+
+ /**
+ * Mark the connection as closed due to an exception.
+ * Sets the {@link #shouldCloseConnection} boolean to true,
+ * and, if it was false earlier, drains the queue before
+ * notifying any waiting objects.
+ * @param e exception which triggered the closure; may be null.
+ */
private synchronized void markClosed(IOException e) {
if (shouldCloseConnection.compareAndSet(false, true)) {
+ Pair<Call, ResponseBuffer> request;
+ while ((request = rpcRequestQueue.poll()) != null) {
+ LOG.debug("Clean {} from RpcRequestQueue.", request.getLeft());
+ }
+ if (queueReservations.get() > 0) {
+ // there's still an active reservation.
+ // either a new put() is about to happen (bad),
+ // or it has happened but the finally {} clause has not been invoked (good).
+ // without knowing which, print a warning message so at least logs on
+ // a deadlock are meaningful.
+ LOG.warn("Possible overlap in queue shutdown and request");
Review Comment:
ok. with the design of owens patch you should just have 1 thread in put() and one worker, but good to show.
--
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: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] virajjasani commented on pull request #5366: HDFS-16853. IPC shutdown failures.
Posted by "virajjasani (via GitHub)" <gi...@apache.org>.
virajjasani commented on PR #5366:
URL: https://github.com/apache/hadoop/pull/5366#issuecomment-1423056620
> it's too dangerous to try and get a fix in here for an RC, so I am proposing rolling back the big change and targeting the 3.3.6 release.
IMO that is reasonable thing to do given we are still not aware whether the impact is only limited to UTs.
--
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: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] virajjasani commented on a diff in pull request #5366: HDFS-16853. IPC shutdown failures.
Posted by "virajjasani (via GitHub)" <gi...@apache.org>.
virajjasani commented on code in PR #5366:
URL: https://github.com/apache/hadoop/pull/5366#discussion_r1099512290
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/Client.java:
##########
@@ -1181,9 +1181,69 @@ public void sendRpcRequest(final Call call)
final ResponseBuffer buf = new ResponseBuffer();
header.writeDelimitedTo(buf);
RpcWritable.wrap(call.rpcRequest).writeTo(buf);
- rpcRequestQueue.put(Pair.of(call, buf));
+ queueIfActive(call, buf);
+ }
+
+ /**
+ * Queue an operation into the request queue,
+ * waiting if necessary for the queue to have a thread to process it.
+ * If the connection is closed, downgrades to a no-op
+ * @param call call to queue
+ * @param buf buffer for response
+ * @throws InterruptedException interrupted while waiting for a free thread.
+ */
+ private void queueIfActive(
+ final Call call,
+ final ResponseBuffer buf) throws InterruptedException {
+ // Get the request queue.
+ // done in a synchronized block to avoid a race condition where
+ // a call is queued after the connection has been closed
+ final SynchronousQueue<Pair<Call, ResponseBuffer>> queue =
+ acquireActiveRequestQueue();
+ if (queue != null) {
+ try {
+ queue.put(Pair.of(call, buf));
+ } finally {
+ // release the reservation afterwards.
+ releaseQueueReservation();
+ }
+ } else {
+ LOG.debug("Discarding queued call as IPC client is stopped");
+ }
+ }
+
+ /**
+ * Get the active rpc request queue.
+ * If the connection is closed, returns null.
+ * This method is synchronized, as are the operations to set
+ * the {@link #shouldCloseConnection} and {@link #running}
+ * atomic booleans, therefore this entire method will complete in the
+ * same block. However, the returned queue may be used outside of
+ * a synchronous block, where this guarantee no longer holds.
+ * A queue reservation counter is used to track this.
+ * Callers MUST invoke {@link #releaseQueueReservation()} afterwards.
+ * @return the queue or null.
+ */
+ private synchronized SynchronousQueue<Pair<Call, ResponseBuffer>> acquireActiveRequestQueue() {
+ if (shouldCloseConnection.get() || !running.get()) {
Review Comment:
This would be too much for accessing atomic boolean
--
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: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] steveloughran commented on pull request #5366: HDFS-16853. IPC shutdown failures.
Posted by "steveloughran (via GitHub)" <gi...@apache.org>.
steveloughran commented on PR #5366:
URL: https://github.com/apache/hadoop/pull/5366#issuecomment-1422702385
see #5369
--
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: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] steveloughran commented on pull request #5366: HDFS-16853. IPC shutdown failures.
Posted by "steveloughran (via GitHub)" <gi...@apache.org>.
steveloughran commented on PR #5366:
URL: https://github.com/apache/hadoop/pull/5366#issuecomment-1421338475
@virajjasani you were near this code...what do you think? @ZanderXu's core patch does the cleanup, but there's still a small window of possible overlap which I can't see how to get rid of through synchronized() blocks. I've got detection, but maybe some semaphore or similar needs to get involved so as to actually block cleanup while other threads are submitting work. dangerous though
--
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: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] steveloughran commented on pull request #5366: HDFS-16853. IPC shutdown failures.
Posted by "steveloughran (via GitHub)" <gi...@apache.org>.
steveloughran commented on PR #5366:
URL: https://github.com/apache/hadoop/pull/5366#issuecomment-1422701626
latest revision has a connection exception raised when queuing on a closed connection, rather than a silent no-op.
this highlights that tests expecting socket retries to work now fail as stuff is going through the closed connection. Either the production code is wrong, or the mockito-based test isn't doing the right thing.
it's too dangerous to try and get a fix in here for an RC, so I am proposing rolling back the big change and targeting the 3.3.6 release.
--
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: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] virajjasani commented on a diff in pull request #5366: HDFS-16853. IPC shutdown failures.
Posted by "virajjasani (via GitHub)" <gi...@apache.org>.
virajjasani commented on code in PR #5366:
URL: https://github.com/apache/hadoop/pull/5366#discussion_r1099431119
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/Client.java:
##########
@@ -1181,9 +1181,69 @@ public void sendRpcRequest(final Call call)
final ResponseBuffer buf = new ResponseBuffer();
header.writeDelimitedTo(buf);
RpcWritable.wrap(call.rpcRequest).writeTo(buf);
- rpcRequestQueue.put(Pair.of(call, buf));
+ queueIfActive(call, buf);
+ }
+
+ /**
+ * Queue an operation into the request queue,
+ * waiting if necessary for the queue to have a thread to process it.
+ * If the connection is closed, downgrades to a no-op
+ * @param call call to queue
+ * @param buf buffer for response
+ * @throws InterruptedException interrupted while waiting for a free thread.
+ */
+ private void queueIfActive(
+ final Call call,
+ final ResponseBuffer buf) throws InterruptedException {
+ // Get the request queue.
+ // done in a synchronized block to avoid a race condition where
+ // a call is queued after the connection has been closed
+ final SynchronousQueue<Pair<Call, ResponseBuffer>> queue =
+ acquireActiveRequestQueue();
+ if (queue != null) {
+ try {
+ queue.put(Pair.of(call, buf));
+ } finally {
+ // release the reservation afterwards.
+ releaseQueueReservation();
+ }
+ } else {
+ LOG.debug("Discarding queued call as IPC client is stopped");
+ }
+ }
+
+ /**
+ * Get the active rpc request queue.
+ * If the connection is closed, returns null.
+ * This method is synchronized, as are the operations to set
+ * the {@link #shouldCloseConnection} and {@link #running}
+ * atomic booleans, therefore this entire method will complete in the
+ * same block. However, the returned queue may be used outside of
+ * a synchronous block, where this guarantee no longer holds.
+ * A queue reservation counter is used to track this.
+ * Callers MUST invoke {@link #releaseQueueReservation()} afterwards.
+ * @return the queue or null.
+ */
+ private synchronized SynchronousQueue<Pair<Call, ResponseBuffer>> acquireActiveRequestQueue() {
+ if (shouldCloseConnection.get() || !running.get()) {
Review Comment:
We do not need additional synchronization on `putLock` object while accessing `running` correct?
Edit: This would be too much for accessing atomic boolean.
--
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: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] steveloughran commented on a diff in pull request #5366: HDFS-16853. IPC shutdown failures.
Posted by "steveloughran (via GitHub)" <gi...@apache.org>.
steveloughran commented on code in PR #5366:
URL: https://github.com/apache/hadoop/pull/5366#discussion_r1099987282
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/Client.java:
##########
@@ -1181,9 +1181,69 @@ public void sendRpcRequest(final Call call)
final ResponseBuffer buf = new ResponseBuffer();
header.writeDelimitedTo(buf);
RpcWritable.wrap(call.rpcRequest).writeTo(buf);
- rpcRequestQueue.put(Pair.of(call, buf));
+ queueIfActive(call, buf);
+ }
+
+ /**
+ * Queue an operation into the request queue,
+ * waiting if necessary for the queue to have a thread to process it.
+ * If the connection is closed, downgrades to a no-op
+ * @param call call to queue
+ * @param buf buffer for response
+ * @throws InterruptedException interrupted while waiting for a free thread.
+ */
+ private void queueIfActive(
+ final Call call,
+ final ResponseBuffer buf) throws InterruptedException {
+ // Get the request queue.
+ // done in a synchronized block to avoid a race condition where
+ // a call is queued after the connection has been closed
+ final SynchronousQueue<Pair<Call, ResponseBuffer>> queue =
+ acquireActiveRequestQueue();
+ if (queue != null) {
+ try {
+ queue.put(Pair.of(call, buf));
+ } finally {
+ // release the reservation afterwards.
+ releaseQueueReservation();
+ }
+ } else {
+ LOG.debug("Discarding queued call as IPC client is stopped");
+ }
+ }
+
+ /**
+ * Get the active rpc request queue.
+ * If the connection is closed, returns null.
+ * This method is synchronized, as are the operations to set
+ * the {@link #shouldCloseConnection} and {@link #running}
+ * atomic booleans, therefore this entire method will complete in the
+ * same block. However, the returned queue may be used outside of
+ * a synchronous block, where this guarantee no longer holds.
+ * A queue reservation counter is used to track this.
+ * Callers MUST invoke {@link #releaseQueueReservation()} afterwards.
+ * @return the queue or null.
+ */
+ private synchronized SynchronousQueue<Pair<Call, ResponseBuffer>> acquireActiveRequestQueue() {
+ if (shouldCloseConnection.get() || !running.get()) {
+ LOG.debug("IPC client is stopped");
Review Comment:
i was thinking that myself
--
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: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] hadoop-yetus commented on pull request #5366: HDFS-16853. IPC shutdown failures.
Posted by "hadoop-yetus (via GitHub)" <gi...@apache.org>.
hadoop-yetus commented on PR #5366:
URL: https://github.com/apache/hadoop/pull/5366#issuecomment-1421621921
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|:----:|----------:|--------:|:--------:|:-------:|
| +0 :ok: | reexec | 0m 51s | | Docker mode activated. |
|||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | | No case conflicting files found. |
| +0 :ok: | codespell | 0m 0s | | codespell was not available. |
| +0 :ok: | detsecrets | 0m 0s | | detect-secrets was not available. |
| +1 :green_heart: | @author | 0m 0s | | The patch does not contain any @author tags. |
| -1 :x: | test4tests | 0m 0s | | The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. |
|||| _ trunk Compile Tests _ |
| +1 :green_heart: | mvninstall | 48m 22s | | trunk passed |
| +1 :green_heart: | compile | 25m 20s | | trunk passed with JDK Ubuntu-11.0.17+8-post-Ubuntu-1ubuntu220.04 |
| +1 :green_heart: | compile | 21m 38s | | trunk passed with JDK Private Build-1.8.0_352-8u352-ga-1~20.04-b08 |
| +1 :green_heart: | checkstyle | 1m 7s | | trunk passed |
| +1 :green_heart: | mvnsite | 1m 38s | | trunk passed |
| +1 :green_heart: | javadoc | 1m 8s | | trunk passed with JDK Ubuntu-11.0.17+8-post-Ubuntu-1ubuntu220.04 |
| +1 :green_heart: | javadoc | 0m 41s | | trunk passed with JDK Private Build-1.8.0_352-8u352-ga-1~20.04-b08 |
| +1 :green_heart: | spotbugs | 2m 40s | | trunk passed |
| +1 :green_heart: | shadedclient | 28m 10s | | branch has no errors when building and testing our client artifacts. |
|||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 1m 7s | | the patch passed |
| +1 :green_heart: | compile | 24m 30s | | the patch passed with JDK Ubuntu-11.0.17+8-post-Ubuntu-1ubuntu220.04 |
| +1 :green_heart: | javac | 24m 30s | | the patch passed |
| +1 :green_heart: | compile | 21m 41s | | the patch passed with JDK Private Build-1.8.0_352-8u352-ga-1~20.04-b08 |
| +1 :green_heart: | javac | 21m 41s | | the patch passed |
| +1 :green_heart: | blanks | 0m 0s | | The patch has no blanks issues. |
| +1 :green_heart: | checkstyle | 1m 0s | | the patch passed |
| +1 :green_heart: | mvnsite | 1m 35s | | the patch passed |
| +1 :green_heart: | javadoc | 0m 58s | | the patch passed with JDK Ubuntu-11.0.17+8-post-Ubuntu-1ubuntu220.04 |
| +1 :green_heart: | javadoc | 0m 41s | | the patch passed with JDK Private Build-1.8.0_352-8u352-ga-1~20.04-b08 |
| +1 :green_heart: | spotbugs | 2m 41s | | the patch passed |
| +1 :green_heart: | shadedclient | 28m 28s | | patch has no errors when building and testing our client artifacts. |
|||| _ Other Tests _ |
| +1 :green_heart: | unit | 18m 31s | | hadoop-common in the patch passed. |
| +1 :green_heart: | asflicense | 1m 6s | | The patch does not generate ASF License warnings. |
| | | 233m 44s | | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.42 ServerAPI=1.42 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5366/1/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hadoop/pull/5366 |
| Optional Tests | dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell detsecrets |
| uname | Linux 8ee5018a3bdc 4.15.0-200-generic #211-Ubuntu SMP Thu Nov 24 18:16:04 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/bin/hadoop.sh |
| git revision | trunk / d7a515e1ac4de6aa6f99b2e9fd8a3f6994cae764 |
| Default Java | Private Build-1.8.0_352-8u352-ga-1~20.04-b08 |
| Multi-JDK versions | /usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.17+8-post-Ubuntu-1ubuntu220.04 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_352-8u352-ga-1~20.04-b08 |
| Test Results | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5366/1/testReport/ |
| Max. process+thread count | 1235 (vs. ulimit of 5500) |
| modules | C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common |
| Console output | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5366/1/console |
| versions | git=2.25.1 maven=3.6.3 spotbugs=4.2.2 |
| Powered by | Apache Yetus 0.14.0 https://yetus.apache.org |
This message was automatically generated.
--
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: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] virajjasani commented on a diff in pull request #5366: HDFS-16853. IPC shutdown failures.
Posted by "virajjasani (via GitHub)" <gi...@apache.org>.
virajjasani commented on code in PR #5366:
URL: https://github.com/apache/hadoop/pull/5366#discussion_r1099397087
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/Client.java:
##########
@@ -1181,9 +1181,69 @@ public void sendRpcRequest(final Call call)
final ResponseBuffer buf = new ResponseBuffer();
header.writeDelimitedTo(buf);
RpcWritable.wrap(call.rpcRequest).writeTo(buf);
- rpcRequestQueue.put(Pair.of(call, buf));
+ queueIfActive(call, buf);
+ }
+
+ /**
+ * Queue an operation into the request queue,
+ * waiting if necessary for the queue to have a thread to process it.
+ * If the connection is closed, downgrades to a no-op
+ * @param call call to queue
+ * @param buf buffer for response
+ * @throws InterruptedException interrupted while waiting for a free thread.
+ */
+ private void queueIfActive(
+ final Call call,
+ final ResponseBuffer buf) throws InterruptedException {
+ // Get the request queue.
+ // done in a synchronized block to avoid a race condition where
+ // a call is queued after the connection has been closed
+ final SynchronousQueue<Pair<Call, ResponseBuffer>> queue =
+ acquireActiveRequestQueue();
+ if (queue != null) {
+ try {
+ queue.put(Pair.of(call, buf));
+ } finally {
+ // release the reservation afterwards.
+ releaseQueueReservation();
+ }
+ } else {
+ LOG.debug("Discarding queued call as IPC client is stopped");
+ }
+ }
+
+ /**
+ * Get the active rpc request queue.
+ * If the connection is closed, returns null.
+ * This method is synchronized, as are the operations to set
+ * the {@link #shouldCloseConnection} and {@link #running}
+ * atomic booleans, therefore this entire method will complete in the
+ * same block. However, the returned queue may be used outside of
+ * a synchronous block, where this guarantee no longer holds.
+ * A queue reservation counter is used to track this.
+ * Callers MUST invoke {@link #releaseQueueReservation()} afterwards.
+ * @return the queue or null.
+ */
+ private synchronized SynchronousQueue<Pair<Call, ResponseBuffer>> acquireActiveRequestQueue() {
+ if (shouldCloseConnection.get() || !running.get()) {
+ LOG.debug("IPC client is stopped");
Review Comment:
nit: `LOG.debug("IPC client {} is stopped", this)` would be great
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/Client.java:
##########
@@ -1241,9 +1301,28 @@ private void receiveRpcResponse() {
markClosed(e);
}
}
-
+
+ /**
+ * Mark the connection as closed due to an exception.
+ * Sets the {@link #shouldCloseConnection} boolean to true,
+ * and, if it was false earlier, drains the queue before
+ * notifying any waiting objects.
+ * @param e exception which triggered the closure; may be null.
+ */
private synchronized void markClosed(IOException e) {
if (shouldCloseConnection.compareAndSet(false, true)) {
+ Pair<Call, ResponseBuffer> request;
+ while ((request = rpcRequestQueue.poll()) != null) {
+ LOG.debug("Clean {} from RpcRequestQueue.", request.getLeft());
+ }
+ if (queueReservations.get() > 0) {
+ // there's still an active reservation.
+ // either a new put() is about to happen (bad),
+ // or it has happened but the finally {} clause has not been invoked (good).
+ // without knowing which, print a warning message so at least logs on
+ // a deadlock are meaningful.
+ LOG.warn("Possible overlap in queue shutdown and request");
Review Comment:
We can also print `queueReservations.get()` with this log
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/Client.java:
##########
@@ -1181,9 +1181,69 @@ public void sendRpcRequest(final Call call)
final ResponseBuffer buf = new ResponseBuffer();
header.writeDelimitedTo(buf);
RpcWritable.wrap(call.rpcRequest).writeTo(buf);
- rpcRequestQueue.put(Pair.of(call, buf));
+ queueIfActive(call, buf);
+ }
+
+ /**
+ * Queue an operation into the request queue,
+ * waiting if necessary for the queue to have a thread to process it.
+ * If the connection is closed, downgrades to a no-op
+ * @param call call to queue
+ * @param buf buffer for response
+ * @throws InterruptedException interrupted while waiting for a free thread.
+ */
+ private void queueIfActive(
+ final Call call,
+ final ResponseBuffer buf) throws InterruptedException {
+ // Get the request queue.
+ // done in a synchronized block to avoid a race condition where
+ // a call is queued after the connection has been closed
+ final SynchronousQueue<Pair<Call, ResponseBuffer>> queue =
+ acquireActiveRequestQueue();
+ if (queue != null) {
+ try {
+ queue.put(Pair.of(call, buf));
+ } finally {
+ // release the reservation afterwards.
+ releaseQueueReservation();
+ }
+ } else {
+ LOG.debug("Discarding queued call as IPC client is stopped");
+ }
+ }
+
+ /**
+ * Get the active rpc request queue.
+ * If the connection is closed, returns null.
+ * This method is synchronized, as are the operations to set
+ * the {@link #shouldCloseConnection} and {@link #running}
+ * atomic booleans, therefore this entire method will complete in the
+ * same block. However, the returned queue may be used outside of
+ * a synchronous block, where this guarantee no longer holds.
+ * A queue reservation counter is used to track this.
+ * Callers MUST invoke {@link #releaseQueueReservation()} afterwards.
+ * @return the queue or null.
+ */
+ private synchronized SynchronousQueue<Pair<Call, ResponseBuffer>> acquireActiveRequestQueue() {
+ if (shouldCloseConnection.get() || !running.get()) {
Review Comment:
We do not need additional synchronization on `putLock` object while accessing `running` correct?
--
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: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] steveloughran commented on pull request #5366: HDFS-16853. IPC shutdown failures.
Posted by "steveloughran (via GitHub)" <gi...@apache.org>.
steveloughran commented on PR #5366:
URL: https://github.com/apache/hadoop/pull/5366#issuecomment-1421341949
@xkrogen thoughts?
--
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: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] steveloughran closed pull request #5366: HDFS-16853. IPC shutdown failures.
Posted by "steveloughran (via GitHub)" <gi...@apache.org>.
steveloughran closed pull request #5366: HDFS-16853. IPC shutdown failures.
URL: https://github.com/apache/hadoop/pull/5366
--
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: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org