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 GitBox <gi...@apache.org> on 2021/09/11 12:19:26 UTC
[GitHub] [hadoop] pbacsko opened a new pull request #3423: HADOOP-17905. Modify Text.ensureCapacity() to efficiently max out the…
pbacsko opened a new pull request #3423:
URL: https://github.com/apache/hadoop/pull/3423
… backing array size
Change-Id: I3cfae85000c5fa7aa86c40c2d7efa282958178bf
<!--
Thanks for sending a pull request!
1. If this is your first time, please read our contributor guidelines: https://cwiki.apache.org/confluence/display/HADOOP/How+To+Contribute
2. Make sure your PR title starts with JIRA issue id, e.g., 'HADOOP-17799. Your PR title ...'.
-->
### Description of PR
Allow org.apache.hadoop.io.Text to expand the underlying byte array to a safe maximum.
### How was this patch tested?
1. Ran unit tests
2. Ran a MapReduce job where a single mapper processed a text file with a size of 1.8G with no line feeds. Array expansion was verified with extra printouts.
### 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] goiri commented on a change in pull request #3423: HADOOP-17905. Modify Text.ensureCapacity() to efficiently max out the…
Posted by GitBox <gi...@apache.org>.
goiri commented on a change in pull request #3423:
URL: https://github.com/apache/hadoop/pull/3423#discussion_r706639391
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/Text.java
##########
@@ -73,6 +73,10 @@ protected CharsetDecoder initialValue() {
}
};
+ // max size of the byte array, seems to be a safe choice for multiple VMs
+ // (see ArrayList.MAX_ARRAY_SIZE)
Review comment:
Where is this ArrayList.MAX_ARRAY_SIZE?
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/Text.java
##########
@@ -73,6 +73,10 @@ protected CharsetDecoder initialValue() {
}
};
+ // max size of the byte array, seems to be a safe choice for multiple VMs
Review comment:
Multiple VMs?
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/Text.java
##########
@@ -301,9 +305,18 @@ public void clear() {
*/
private boolean ensureCapacity(final int capacity) {
if (bytes.length < capacity) {
+ // use long to allow overflow
+ long tmpLength = bytes.length;
+ long tmpCapacity = capacity;
+
// Try to expand the backing array by the factor of 1.5x
- // (by taking the current size + diving it by half)
- int targetSize = Math.max(capacity, bytes.length + (bytes.length >> 1));
+ // (by taking the current size + diving it by half).
+ //
+ // If the calculated value is beyond the size
+ // limit, we cap it to ARRAY_MAX_SIZE
+ int targetSize = (int)Math.min(ARRAY_MAX_SIZE,
Review comment:
Why do we need the cast to int by the way?
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/Text.java
##########
@@ -301,9 +305,18 @@ public void clear() {
*/
private boolean ensureCapacity(final int capacity) {
if (bytes.length < capacity) {
+ // use long to allow overflow
+ long tmpLength = bytes.length;
+ long tmpCapacity = capacity;
+
// Try to expand the backing array by the factor of 1.5x
- // (by taking the current size + diving it by half)
- int targetSize = Math.max(capacity, bytes.length + (bytes.length >> 1));
+ // (by taking the current size + diving it by half).
+ //
+ // If the calculated value is beyond the size
+ // limit, we cap it to ARRAY_MAX_SIZE
+ int targetSize = (int)Math.min(ARRAY_MAX_SIZE,
Review comment:
I would separate it into two lines.
int targetSize = Math.max(tmpCapacity, tmpLength + (tmpLength >> 1));
targetSize = (int)Math.min(ARRAY_MAX_SIZE, targetSize);
--
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 #3423: HADOOP-17905. Modify Text.ensureCapacity() to efficiently max out the…
Posted by GitBox <gi...@apache.org>.
hadoop-yetus commented on pull request #3423:
URL: https://github.com/apache/hadoop/pull/3423#issuecomment-920209887
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|:----:|----------:|--------:|:--------:|:-------:|
| +0 :ok: | reexec | 1m 5s | | Docker mode activated. |
|||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | | No case conflicting files found. |
| +0 :ok: | codespell | 0m 0s | | codespell 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 | 33m 50s | | trunk passed |
| +1 :green_heart: | compile | 23m 8s | | trunk passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04 |
| +1 :green_heart: | compile | 19m 29s | | trunk passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 |
| +1 :green_heart: | checkstyle | 1m 0s | | trunk passed |
| +1 :green_heart: | mvnsite | 1m 32s | | trunk passed |
| +1 :green_heart: | javadoc | 1m 1s | | trunk passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04 |
| +1 :green_heart: | javadoc | 1m 36s | | trunk passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 |
| +1 :green_heart: | spotbugs | 2m 23s | | trunk passed |
| +1 :green_heart: | shadedclient | 35m 44s | | branch has no errors when building and testing our client artifacts. |
|||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 0m 56s | | the patch passed |
| +1 :green_heart: | compile | 22m 13s | | the patch passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04 |
| +1 :green_heart: | javac | 22m 13s | | the patch passed |
| +1 :green_heart: | compile | 19m 28s | | the patch passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 |
| +1 :green_heart: | javac | 19m 28s | | 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 30s | | the patch passed |
| +1 :green_heart: | javadoc | 1m 0s | | the patch passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04 |
| +1 :green_heart: | javadoc | 1m 34s | | the patch passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 |
| +1 :green_heart: | spotbugs | 2m 34s | | the patch passed |
| +1 :green_heart: | shadedclient | 35m 48s | | patch has no errors when building and testing our client artifacts. |
|||| _ Other Tests _ |
| +1 :green_heart: | unit | 17m 0s | | hadoop-common in the patch passed. |
| +1 :green_heart: | asflicense | 0m 49s | | The patch does not generate ASF License warnings. |
| | | 225m 9s | | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3423/4/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hadoop/pull/3423 |
| Optional Tests | dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell |
| uname | Linux b67dd1b46a84 4.15.0-142-generic #146-Ubuntu SMP Tue Apr 13 01:11:19 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/bin/hadoop.sh |
| git revision | trunk / d82e53f9626373492899ce413ed64cb2ad9bb1a8 |
| Default Java | Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 |
| Multi-JDK versions | /usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 |
| Test Results | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3423/4/testReport/ |
| Max. process+thread count | 2528 (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-3423/4/console |
| versions | git=2.25.1 maven=3.6.3 spotbugs=4.2.2 |
| Powered by | Apache Yetus 0.14.0-SNAPSHOT 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] goiri commented on a change in pull request #3423: HADOOP-17905. Modify Text.ensureCapacity() to efficiently max out the…
Posted by GitBox <gi...@apache.org>.
goiri commented on a change in pull request #3423:
URL: https://github.com/apache/hadoop/pull/3423#discussion_r708538792
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/Text.java
##########
@@ -301,9 +305,18 @@ public void clear() {
*/
private boolean ensureCapacity(final int capacity) {
if (bytes.length < capacity) {
+ // use long to allow overflow
+ long tmpLength = bytes.length;
+ long tmpCapacity = capacity;
+
// Try to expand the backing array by the factor of 1.5x
- // (by taking the current size + diving it by half)
- int targetSize = Math.max(capacity, bytes.length + (bytes.length >> 1));
+ // (by taking the current size + diving it by half).
+ //
+ // If the calculated value is beyond the size
+ // limit, we cap it to ARRAY_MAX_SIZE
+ int targetSize = (int)Math.min(ARRAY_MAX_SIZE,
Review comment:
I would prefer to cast inside so it uses Math.max(int, int) and keeps everything as ints.
The splitting for the min and the max is more a matter of readability.
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/Text.java
##########
@@ -73,6 +73,10 @@ protected CharsetDecoder initialValue() {
}
};
+ // max size of the byte array, seems to be a safe choice for multiple VMs
Review comment:
JVMs then
--
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] goiri commented on a change in pull request #3423: HADOOP-17905. Modify Text.ensureCapacity() to efficiently max out the…
Posted by GitBox <gi...@apache.org>.
goiri commented on a change in pull request #3423:
URL: https://github.com/apache/hadoop/pull/3423#discussion_r717995655
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/Text.java
##########
@@ -301,9 +305,18 @@ public void clear() {
*/
private boolean ensureCapacity(final int capacity) {
if (bytes.length < capacity) {
+ // use long to allow overflow
+ long tmpLength = bytes.length;
+ long tmpCapacity = capacity;
+
// Try to expand the backing array by the factor of 1.5x
- // (by taking the current size + diving it by half)
- int targetSize = Math.max(capacity, bytes.length + (bytes.length >> 1));
+ // (by taking the current size + diving it by half).
+ //
+ // If the calculated value is beyond the size
+ // limit, we cap it to ARRAY_MAX_SIZE
+ int targetSize = (int)Math.min(ARRAY_MAX_SIZE,
Review comment:
If we are protecting against overflows, then we should make it explicit:
```
long targetSizeLong = bytes.length + (bytes.length >> 1);
int targetSize = (int)Math.min(targetSizeLong, ARRAY_MAX_SIZE);
targetSize = Math.max(capacity, targetSize);
```
--
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] goiri commented on a change in pull request #3423: HADOOP-17905. Modify Text.ensureCapacity() to efficiently max out the…
Posted by GitBox <gi...@apache.org>.
goiri commented on a change in pull request #3423:
URL: https://github.com/apache/hadoop/pull/3423#discussion_r717032871
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/Text.java
##########
@@ -301,9 +305,18 @@ public void clear() {
*/
private boolean ensureCapacity(final int capacity) {
if (bytes.length < capacity) {
+ // use long to allow overflow
+ long tmpLength = bytes.length;
+ long tmpCapacity = capacity;
+
// Try to expand the backing array by the factor of 1.5x
- // (by taking the current size + diving it by half)
- int targetSize = Math.max(capacity, bytes.length + (bytes.length >> 1));
+ // (by taking the current size + diving it by half).
+ //
+ // If the calculated value is beyond the size
+ // limit, we cap it to ARRAY_MAX_SIZE
+ int targetSize = (int)Math.min(ARRAY_MAX_SIZE,
Review comment:
I think the following is easier to read and less casting required:
```
int targetSize = (int) bytes.length + (bytes.length >> 1);
targetSize = Math.max(capacity, targetSize);
targetSize = Math.min(ARRAY_MAX_SIZE, targetSize);
```
--
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 #3423: HADOOP-17905. Modify Text.ensureCapacity() to efficiently max out the…
Posted by GitBox <gi...@apache.org>.
hadoop-yetus commented on pull request #3423:
URL: https://github.com/apache/hadoop/pull/3423#issuecomment-917427533
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|:----:|----------:|--------:|:--------:|:-------:|
| +0 :ok: | reexec | 0m 54s | | Docker mode activated. |
|||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | | No case conflicting files found. |
| +0 :ok: | codespell | 0m 1s | | codespell 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 | 31m 2s | | trunk passed |
| +1 :green_heart: | compile | 21m 17s | | trunk passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04 |
| +1 :green_heart: | compile | 18m 28s | | trunk passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 |
| +1 :green_heart: | checkstyle | 1m 11s | | trunk passed |
| +1 :green_heart: | mvnsite | 1m 38s | | trunk passed |
| +1 :green_heart: | javadoc | 1m 10s | | trunk passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04 |
| +1 :green_heart: | javadoc | 1m 39s | | trunk passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 |
| +1 :green_heart: | spotbugs | 2m 26s | | trunk passed |
| +1 :green_heart: | shadedclient | 15m 47s | | branch has no errors when building and testing our client artifacts. |
|||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 0m 55s | | the patch passed |
| +1 :green_heart: | compile | 21m 23s | | the patch passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04 |
| +1 :green_heart: | javac | 21m 23s | | the patch passed |
| +1 :green_heart: | compile | 19m 39s | | the patch passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 |
| +1 :green_heart: | javac | 19m 39s | | the patch passed |
| +1 :green_heart: | blanks | 0m 0s | | The patch has no blanks issues. |
| +1 :green_heart: | checkstyle | 1m 14s | | the patch passed |
| +1 :green_heart: | mvnsite | 1m 51s | | the patch passed |
| +1 :green_heart: | javadoc | 1m 15s | | the patch passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04 |
| +1 :green_heart: | javadoc | 1m 47s | | the patch passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 |
| +1 :green_heart: | spotbugs | 3m 21s | | the patch passed |
| +1 :green_heart: | shadedclient | 20m 57s | | patch has no errors when building and testing our client artifacts. |
|||| _ Other Tests _ |
| +1 :green_heart: | unit | 19m 26s | | hadoop-common in the patch passed. |
| +1 :green_heart: | asflicense | 0m 58s | | The patch does not generate ASF License warnings. |
| | | 188m 27s | | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3423/2/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hadoop/pull/3423 |
| Optional Tests | dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell |
| uname | Linux 1525d4527b51 4.15.0-151-generic #157-Ubuntu SMP Fri Jul 9 23:07:57 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/bin/hadoop.sh |
| git revision | trunk / ba11e4dcc69491d8afcb9d046147d740a6d2bda9 |
| Default Java | Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 |
| Multi-JDK versions | /usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 |
| Test Results | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3423/2/testReport/ |
| Max. process+thread count | 3149 (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-3423/2/console |
| versions | git=2.25.1 maven=3.6.3 spotbugs=4.2.2 |
| Powered by | Apache Yetus 0.14.0-SNAPSHOT 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] bharatviswa504 commented on a change in pull request #3423: HADOOP-17905. Modify Text.ensureCapacity() to efficiently max out the…
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #3423:
URL: https://github.com/apache/hadoop/pull/3423#discussion_r717760340
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/Text.java
##########
@@ -301,9 +305,18 @@ public void clear() {
*/
private boolean ensureCapacity(final int capacity) {
if (bytes.length < capacity) {
+ // use long to allow overflow
+ long tmpLength = bytes.length;
+ long tmpCapacity = capacity;
+
// Try to expand the backing array by the factor of 1.5x
- // (by taking the current size + diving it by half)
- int targetSize = Math.max(capacity, bytes.length + (bytes.length >> 1));
+ // (by taking the current size + diving it by half).
+ //
+ // If the calculated value is beyond the size
+ // limit, we cap it to ARRAY_MAX_SIZE
+ int targetSize = (int)Math.min(ARRAY_MAX_SIZE,
Review comment:
With the explanation it makes sense to me.
--
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] pbacsko commented on a change in pull request #3423: HADOOP-17905. Modify Text.ensureCapacity() to efficiently max out the…
Posted by GitBox <gi...@apache.org>.
pbacsko commented on a change in pull request #3423:
URL: https://github.com/apache/hadoop/pull/3423#discussion_r717676453
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/Text.java
##########
@@ -301,9 +305,18 @@ public void clear() {
*/
private boolean ensureCapacity(final int capacity) {
if (bytes.length < capacity) {
+ // use long to allow overflow
+ long tmpLength = bytes.length;
+ long tmpCapacity = capacity;
+
// Try to expand the backing array by the factor of 1.5x
- // (by taking the current size + diving it by half)
- int targetSize = Math.max(capacity, bytes.length + (bytes.length >> 1));
+ // (by taking the current size + diving it by half).
+ //
+ // If the calculated value is beyond the size
+ // limit, we cap it to ARRAY_MAX_SIZE
+ int targetSize = (int)Math.min(ARRAY_MAX_SIZE,
Review comment:
The problem is that `bytes.length + (bytes.length >> 1);` might overflow and end up being negative which results in choosing `capacity` all the time instead of `ARRAY_MAX_SIZE`. So we either temporarily store this as `long` and cast back to `int` or we directly check if `targetSize` is negative after `int targetSize = (int) bytes.length + (bytes.length >> 1);`.
--
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 #3423: HADOOP-17905. Modify Text.ensureCapacity() to efficiently max out the…
Posted by GitBox <gi...@apache.org>.
hadoop-yetus commented on pull request #3423:
URL: https://github.com/apache/hadoop/pull/3423#issuecomment-930199207
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|:----:|----------:|--------:|:--------:|:-------:|
| +0 :ok: | reexec | 1m 0s | | Docker mode activated. |
|||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | | No case conflicting files found. |
| +0 :ok: | codespell | 0m 0s | | codespell 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 | 36m 14s | | trunk passed |
| +1 :green_heart: | compile | 27m 55s | | trunk passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04 |
| +1 :green_heart: | compile | 23m 18s | | trunk passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 |
| +1 :green_heart: | checkstyle | 1m 10s | | trunk passed |
| +1 :green_heart: | mvnsite | 1m 57s | | trunk passed |
| +1 :green_heart: | javadoc | 1m 16s | | trunk passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04 |
| +1 :green_heart: | javadoc | 1m 50s | | trunk passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 |
| +1 :green_heart: | spotbugs | 2m 49s | | trunk passed |
| +1 :green_heart: | shadedclient | 27m 20s | | branch has no errors when building and testing our client artifacts. |
|||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 1m 11s | | the patch passed |
| +1 :green_heart: | compile | 27m 12s | | the patch passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04 |
| +1 :green_heart: | javac | 27m 12s | | the patch passed |
| +1 :green_heart: | compile | 23m 6s | | the patch passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 |
| +1 :green_heart: | javac | 23m 6s | | the patch passed |
| +1 :green_heart: | blanks | 0m 0s | | The patch has no blanks issues. |
| +1 :green_heart: | checkstyle | 1m 1s | | the patch passed |
| +1 :green_heart: | mvnsite | 1m 39s | | the patch passed |
| +1 :green_heart: | javadoc | 1m 15s | | the patch passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04 |
| +1 :green_heart: | javadoc | 1m 47s | | the patch passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 |
| +1 :green_heart: | spotbugs | 3m 2s | | the patch passed |
| +1 :green_heart: | shadedclient | 26m 58s | | patch has no errors when building and testing our client artifacts. |
|||| _ Other Tests _ |
| +1 :green_heart: | unit | 18m 28s | | hadoop-common in the patch passed. |
| +1 :green_heart: | asflicense | 0m 57s | | The patch does not generate ASF License warnings. |
| | | 230m 55s | | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3423/5/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hadoop/pull/3423 |
| Optional Tests | dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell |
| uname | Linux 6b9deed95f06 4.15.0-142-generic #146-Ubuntu SMP Tue Apr 13 01:11:19 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/bin/hadoop.sh |
| git revision | trunk / 4a89581189e9b510823aaa1b3962221a3e3356f0 |
| Default Java | Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 |
| Multi-JDK versions | /usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 |
| Test Results | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3423/5/testReport/ |
| Max. process+thread count | 2014 (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-3423/5/console |
| versions | git=2.25.1 maven=3.6.3 spotbugs=4.2.2 |
| Powered by | Apache Yetus 0.14.0-SNAPSHOT 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] hadoop-yetus commented on pull request #3423: HADOOP-17905. Modify Text.ensureCapacity() to efficiently max out the…
Posted by GitBox <gi...@apache.org>.
hadoop-yetus commented on pull request #3423:
URL: https://github.com/apache/hadoop/pull/3423#issuecomment-930199207
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|:----:|----------:|--------:|:--------:|:-------:|
| +0 :ok: | reexec | 1m 0s | | Docker mode activated. |
|||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | | No case conflicting files found. |
| +0 :ok: | codespell | 0m 0s | | codespell 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 | 36m 14s | | trunk passed |
| +1 :green_heart: | compile | 27m 55s | | trunk passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04 |
| +1 :green_heart: | compile | 23m 18s | | trunk passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 |
| +1 :green_heart: | checkstyle | 1m 10s | | trunk passed |
| +1 :green_heart: | mvnsite | 1m 57s | | trunk passed |
| +1 :green_heart: | javadoc | 1m 16s | | trunk passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04 |
| +1 :green_heart: | javadoc | 1m 50s | | trunk passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 |
| +1 :green_heart: | spotbugs | 2m 49s | | trunk passed |
| +1 :green_heart: | shadedclient | 27m 20s | | branch has no errors when building and testing our client artifacts. |
|||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 1m 11s | | the patch passed |
| +1 :green_heart: | compile | 27m 12s | | the patch passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04 |
| +1 :green_heart: | javac | 27m 12s | | the patch passed |
| +1 :green_heart: | compile | 23m 6s | | the patch passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 |
| +1 :green_heart: | javac | 23m 6s | | the patch passed |
| +1 :green_heart: | blanks | 0m 0s | | The patch has no blanks issues. |
| +1 :green_heart: | checkstyle | 1m 1s | | the patch passed |
| +1 :green_heart: | mvnsite | 1m 39s | | the patch passed |
| +1 :green_heart: | javadoc | 1m 15s | | the patch passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04 |
| +1 :green_heart: | javadoc | 1m 47s | | the patch passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 |
| +1 :green_heart: | spotbugs | 3m 2s | | the patch passed |
| +1 :green_heart: | shadedclient | 26m 58s | | patch has no errors when building and testing our client artifacts. |
|||| _ Other Tests _ |
| +1 :green_heart: | unit | 18m 28s | | hadoop-common in the patch passed. |
| +1 :green_heart: | asflicense | 0m 57s | | The patch does not generate ASF License warnings. |
| | | 230m 55s | | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3423/5/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hadoop/pull/3423 |
| Optional Tests | dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell |
| uname | Linux 6b9deed95f06 4.15.0-142-generic #146-Ubuntu SMP Tue Apr 13 01:11:19 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/bin/hadoop.sh |
| git revision | trunk / 4a89581189e9b510823aaa1b3962221a3e3356f0 |
| Default Java | Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 |
| Multi-JDK versions | /usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 |
| Test Results | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3423/5/testReport/ |
| Max. process+thread count | 2014 (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-3423/5/console |
| versions | git=2.25.1 maven=3.6.3 spotbugs=4.2.2 |
| Powered by | Apache Yetus 0.14.0-SNAPSHOT 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] pbacsko commented on a change in pull request #3423: HADOOP-17905. Modify Text.ensureCapacity() to efficiently max out the…
Posted by GitBox <gi...@apache.org>.
pbacsko commented on a change in pull request #3423:
URL: https://github.com/apache/hadoop/pull/3423#discussion_r706827825
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/Text.java
##########
@@ -73,6 +73,10 @@ protected CharsetDecoder initialValue() {
}
};
+ // max size of the byte array, seems to be a safe choice for multiple VMs
Review comment:
Maybe I should have written different kind of VMs (OpenJDK, HotSpot, etc). It's more like a practical value that will likely work under different versions. Some details: https://programming.guide/java/array-maximum-length.html. If this comment is confusing, I can remove it.
--
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] pbacsko commented on a change in pull request #3423: HADOOP-17905. Modify Text.ensureCapacity() to efficiently max out the…
Posted by GitBox <gi...@apache.org>.
pbacsko commented on a change in pull request #3423:
URL: https://github.com/apache/hadoop/pull/3423#discussion_r714916621
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/Text.java
##########
@@ -301,9 +305,18 @@ public void clear() {
*/
private boolean ensureCapacity(final int capacity) {
if (bytes.length < capacity) {
+ // use long to allow overflow
+ long tmpLength = bytes.length;
+ long tmpCapacity = capacity;
+
// Try to expand the backing array by the factor of 1.5x
- // (by taking the current size + diving it by half)
- int targetSize = Math.max(capacity, bytes.length + (bytes.length >> 1));
+ // (by taking the current size + diving it by half).
+ //
+ // If the calculated value is beyond the size
+ // limit, we cap it to ARRAY_MAX_SIZE
+ int targetSize = (int)Math.min(ARRAY_MAX_SIZE,
Review comment:
@goiri could you comment on the latest changes?
--
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] bharatviswa504 commented on a change in pull request #3423: HADOOP-17905. Modify Text.ensureCapacity() to efficiently max out the…
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #3423:
URL: https://github.com/apache/hadoop/pull/3423#discussion_r717760340
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/Text.java
##########
@@ -301,9 +305,18 @@ public void clear() {
*/
private boolean ensureCapacity(final int capacity) {
if (bytes.length < capacity) {
+ // use long to allow overflow
+ long tmpLength = bytes.length;
+ long tmpCapacity = capacity;
+
// Try to expand the backing array by the factor of 1.5x
- // (by taking the current size + diving it by half)
- int targetSize = Math.max(capacity, bytes.length + (bytes.length >> 1));
+ // (by taking the current size + diving it by half).
+ //
+ // If the calculated value is beyond the size
+ // limit, we cap it to ARRAY_MAX_SIZE
+ int targetSize = (int)Math.min(ARRAY_MAX_SIZE,
Review comment:
With the explanation it makes sense to me.
--
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] pbacsko commented on a change in pull request #3423: HADOOP-17905. Modify Text.ensureCapacity() to efficiently max out the…
Posted by GitBox <gi...@apache.org>.
pbacsko commented on a change in pull request #3423:
URL: https://github.com/apache/hadoop/pull/3423#discussion_r717676453
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/Text.java
##########
@@ -301,9 +305,18 @@ public void clear() {
*/
private boolean ensureCapacity(final int capacity) {
if (bytes.length < capacity) {
+ // use long to allow overflow
+ long tmpLength = bytes.length;
+ long tmpCapacity = capacity;
+
// Try to expand the backing array by the factor of 1.5x
- // (by taking the current size + diving it by half)
- int targetSize = Math.max(capacity, bytes.length + (bytes.length >> 1));
+ // (by taking the current size + diving it by half).
+ //
+ // If the calculated value is beyond the size
+ // limit, we cap it to ARRAY_MAX_SIZE
+ int targetSize = (int)Math.min(ARRAY_MAX_SIZE,
Review comment:
The problem is that `bytes.length + (bytes.length >> 1);` might overflow and end up being negative which results in choosing `capacity` all the time instead of `ARRAY_MAX_SIZE`. So we either temporarily store this as `long` and cast back to `int` or we directly check if `targetSize` is negative after `int targetSize = (int) bytes.length + (bytes.length >> 1);`.
--
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] pbacsko commented on a change in pull request #3423: HADOOP-17905. Modify Text.ensureCapacity() to efficiently max out the…
Posted by GitBox <gi...@apache.org>.
pbacsko commented on a change in pull request #3423:
URL: https://github.com/apache/hadoop/pull/3423#discussion_r706827825
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/Text.java
##########
@@ -73,6 +73,10 @@ protected CharsetDecoder initialValue() {
}
};
+ // max size of the byte array, seems to be a safe choice for multiple VMs
Review comment:
Maybe I should have written different kind of VMs (OpenJDK, HotSpot, etc). It's more like a practical value that will likely work under different versions.
--
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 #3423: HADOOP-17905. Modify Text.ensureCapacity() to efficiently max out the…
Posted by GitBox <gi...@apache.org>.
hadoop-yetus commented on pull request #3423:
URL: https://github.com/apache/hadoop/pull/3423#issuecomment-917611567
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|:----:|----------:|--------:|:--------:|:-------:|
| +0 :ok: | reexec | 0m 56s | | 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: | shelldocs | 0m 0s | | Shelldocs was not available. |
| -1 :x: | @author | 0m 0s | [/results-author.txt](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3423/1/artifact/out/results-author.txt) | The patch appears to contain 2 @author tags which the community has agreed to not allow in code contributions. |
| -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 _ |
| +0 :ok: | mvndep | 12m 43s | | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 22m 47s | | trunk passed |
| +1 :green_heart: | compile | 22m 56s | | trunk passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04 |
| +1 :green_heart: | compile | 19m 30s | | trunk passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 |
| +1 :green_heart: | checkstyle | 3m 49s | | trunk passed |
| +1 :green_heart: | mvnsite | 25m 33s | | trunk passed |
| +1 :green_heart: | javadoc | 7m 51s | | trunk passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04 |
| +1 :green_heart: | javadoc | 7m 51s | | trunk passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 |
| +1 :green_heart: | spotbugs | 36m 49s | | trunk passed |
| +1 :green_heart: | shadedclient | 50m 9s | | branch has no errors when building and testing our client artifacts. |
|||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 21s | | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 23m 29s | | the patch passed |
| +1 :green_heart: | compile | 22m 27s | | the patch passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04 |
| +1 :green_heart: | javac | 22m 27s | | the patch passed |
| +1 :green_heart: | compile | 19m 35s | | the patch passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 |
| +1 :green_heart: | javac | 19m 35s | | the patch passed |
| -1 :x: | blanks | 0m 0s | [/blanks-eol.txt](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3423/1/artifact/out/blanks-eol.txt) | The patch has 14736 line(s) that end in blanks. Use git apply --whitespace=fix <<patch_file>>. Refer https://git-scm.com/docs/git-apply |
| +1 :green_heart: | checkstyle | 3m 47s | | the patch passed |
| +1 :green_heart: | mvnsite | 21m 23s | | the patch passed |
| +1 :green_heart: | shellcheck | 0m 1s | | No new issues. |
| +1 :green_heart: | javadoc | 7m 42s | | the patch passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04 |
| +1 :green_heart: | javadoc | 7m 47s | | the patch passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 |
| +1 :green_heart: | spotbugs | 37m 21s | | the patch passed |
| +1 :green_heart: | shadedclient | 50m 41s | | patch has no errors when building and testing our client artifacts. |
|||| _ Other Tests _ |
| -1 :x: | unit | 1009m 41s | [/patch-unit-root.txt](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3423/1/artifact/out/patch-unit-root.txt) | root in the patch passed. |
| -1 :x: | asflicense | 1m 18s | [/results-asflicense.txt](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3423/1/artifact/out/results-asflicense.txt) | The patch generated 2 ASF License warnings. |
| | | 1351m 58s | | |
| Reason | Tests |
|-------:|:------|
| Failed junit tests | hadoop.hdfs.rbfbalance.TestRouterDistCpProcedure |
| | hadoop.tools.dynamometer.TestDynamometerInfra |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3423/1/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hadoop/pull/3423 |
| Optional Tests | dupname asflicense codespell shellcheck shelldocs compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle |
| uname | Linux bbe67560c19d 4.15.0-147-generic #151-Ubuntu SMP Fri Jun 18 19:21:19 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/bin/hadoop.sh |
| git revision | trunk / fc99e97a629e28fc4fd4fd772da918634dd1824a |
| Default Java | Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 |
| Multi-JDK versions | /usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 |
| Test Results | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3423/1/testReport/ |
| Max. process+thread count | 2220 (vs. ulimit of 5500) |
| modules | C: hadoop-common-project/hadoop-common . U: . |
| Console output | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3423/1/console |
| versions | git=2.25.1 maven=3.6.3 shellcheck=0.7.0 spotbugs=4.2.2 |
| Powered by | Apache Yetus 0.14.0-SNAPSHOT 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] goiri commented on a change in pull request #3423: HADOOP-17905. Modify Text.ensureCapacity() to efficiently max out the…
Posted by GitBox <gi...@apache.org>.
goiri commented on a change in pull request #3423:
URL: https://github.com/apache/hadoop/pull/3423#discussion_r717032871
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/Text.java
##########
@@ -301,9 +305,18 @@ public void clear() {
*/
private boolean ensureCapacity(final int capacity) {
if (bytes.length < capacity) {
+ // use long to allow overflow
+ long tmpLength = bytes.length;
+ long tmpCapacity = capacity;
+
// Try to expand the backing array by the factor of 1.5x
- // (by taking the current size + diving it by half)
- int targetSize = Math.max(capacity, bytes.length + (bytes.length >> 1));
+ // (by taking the current size + diving it by half).
+ //
+ // If the calculated value is beyond the size
+ // limit, we cap it to ARRAY_MAX_SIZE
+ int targetSize = (int)Math.min(ARRAY_MAX_SIZE,
Review comment:
I think the following is easier to read and less casting required:
```
int targetSize = (int) bytes.length + (bytes.length >> 1);
targetSize = Math.max(capacity, targetSize);
targetSize = Math.min(ARRAY_MAX_SIZE, targetSize);
```
--
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] pbacsko commented on a change in pull request #3423: HADOOP-17905. Modify Text.ensureCapacity() to efficiently max out the…
Posted by GitBox <gi...@apache.org>.
pbacsko commented on a change in pull request #3423:
URL: https://github.com/apache/hadoop/pull/3423#discussion_r706827825
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/Text.java
##########
@@ -73,6 +73,10 @@ protected CharsetDecoder initialValue() {
}
};
+ // max size of the byte array, seems to be a safe choice for multiple VMs
Review comment:
Maybe I should have written different kind of VMs (OpenJDK, HotSpot, etc). It's more like a practical value that will likely work under different versions. Some details: https://programming.guide/java/array-maximum-length.html. If this comment is confusing, I can remove it (or perhaps extend it a little bit?).
--
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] pbacsko commented on a change in pull request #3423: HADOOP-17905. Modify Text.ensureCapacity() to efficiently max out the…
Posted by GitBox <gi...@apache.org>.
pbacsko commented on a change in pull request #3423:
URL: https://github.com/apache/hadoop/pull/3423#discussion_r706828659
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/Text.java
##########
@@ -301,9 +305,18 @@ public void clear() {
*/
private boolean ensureCapacity(final int capacity) {
if (bytes.length < capacity) {
+ // use long to allow overflow
+ long tmpLength = bytes.length;
+ long tmpCapacity = capacity;
+
// Try to expand the backing array by the factor of 1.5x
- // (by taking the current size + diving it by half)
- int targetSize = Math.max(capacity, bytes.length + (bytes.length >> 1));
+ // (by taking the current size + diving it by half).
+ //
+ // If the calculated value is beyond the size
+ // limit, we cap it to ARRAY_MAX_SIZE
+ int targetSize = (int)Math.min(ARRAY_MAX_SIZE,
Review comment:
The overloaded Math.max(long, long) will be used, which returns long, so it has to be cast back to an int. Therefore, in your suggestion, targetSize should also be a long (which, again has to be cast to an "int" when you instantiate the array).
--
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] pbacsko commented on a change in pull request #3423: HADOOP-17905. Modify Text.ensureCapacity() to efficiently max out the…
Posted by GitBox <gi...@apache.org>.
pbacsko commented on a change in pull request #3423:
URL: https://github.com/apache/hadoop/pull/3423#discussion_r706827939
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/Text.java
##########
@@ -73,6 +73,10 @@ protected CharsetDecoder initialValue() {
}
};
+ // max size of the byte array, seems to be a safe choice for multiple VMs
+ // (see ArrayList.MAX_ARRAY_SIZE)
Review comment:
Open the implementation of java.util.ArrayList, you'll find it. Also AbstractCollection, HashTable, etc. They all use this value.
--
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] bharatviswa504 merged pull request #3423: HADOOP-17905. Modify Text.ensureCapacity() to efficiently max out the…
Posted by GitBox <gi...@apache.org>.
bharatviswa504 merged pull request #3423:
URL: https://github.com/apache/hadoop/pull/3423
--
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] goiri commented on a change in pull request #3423: HADOOP-17905. Modify Text.ensureCapacity() to efficiently max out the…
Posted by GitBox <gi...@apache.org>.
goiri commented on a change in pull request #3423:
URL: https://github.com/apache/hadoop/pull/3423#discussion_r717995655
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/Text.java
##########
@@ -301,9 +305,18 @@ public void clear() {
*/
private boolean ensureCapacity(final int capacity) {
if (bytes.length < capacity) {
+ // use long to allow overflow
+ long tmpLength = bytes.length;
+ long tmpCapacity = capacity;
+
// Try to expand the backing array by the factor of 1.5x
- // (by taking the current size + diving it by half)
- int targetSize = Math.max(capacity, bytes.length + (bytes.length >> 1));
+ // (by taking the current size + diving it by half).
+ //
+ // If the calculated value is beyond the size
+ // limit, we cap it to ARRAY_MAX_SIZE
+ int targetSize = (int)Math.min(ARRAY_MAX_SIZE,
Review comment:
If we are protecting against overflows, then we should make it explicit:
```
long targetSizeLong = bytes.length + (bytes.length >> 1);
int targetSize = (int)Math.min(targetSizeLong, ARRAY_MAX_SIZE);
targetSize = Math.max(capacity, targetSize);
```
--
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 #3423: HADOOP-17905. Modify Text.ensureCapacity() to efficiently max out the…
Posted by GitBox <gi...@apache.org>.
hadoop-yetus commented on pull request #3423:
URL: https://github.com/apache/hadoop/pull/3423#issuecomment-920207873
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|:----:|----------:|--------:|:--------:|:-------:|
| +0 :ok: | reexec | 0m 54s | | Docker mode activated. |
|||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | | No case conflicting files found. |
| +0 :ok: | codespell | 0m 0s | | codespell 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 | 33m 43s | | trunk passed |
| +1 :green_heart: | compile | 23m 1s | | trunk passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04 |
| +1 :green_heart: | compile | 19m 29s | | trunk passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 |
| +1 :green_heart: | checkstyle | 0m 59s | | trunk passed |
| +1 :green_heart: | mvnsite | 1m 32s | | trunk passed |
| +1 :green_heart: | javadoc | 1m 2s | | trunk passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04 |
| +1 :green_heart: | javadoc | 1m 33s | | trunk passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 |
| +1 :green_heart: | spotbugs | 2m 32s | | trunk passed |
| +1 :green_heart: | shadedclient | 36m 2s | | branch has no errors when building and testing our client artifacts. |
|||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 0m 57s | | the patch passed |
| +1 :green_heart: | compile | 22m 19s | | the patch passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04 |
| +1 :green_heart: | javac | 22m 19s | | the patch passed |
| +1 :green_heart: | compile | 19m 31s | | the patch passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 |
| +1 :green_heart: | javac | 19m 31s | | 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 31s | | the patch passed |
| +1 :green_heart: | javadoc | 1m 0s | | the patch passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04 |
| +1 :green_heart: | javadoc | 1m 41s | | the patch passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 |
| +1 :green_heart: | spotbugs | 2m 35s | | the patch passed |
| +1 :green_heart: | shadedclient | 35m 29s | | patch has no errors when building and testing our client artifacts. |
|||| _ Other Tests _ |
| +1 :green_heart: | unit | 16m 59s | | hadoop-common in the patch passed. |
| +1 :green_heart: | asflicense | 0m 48s | | The patch does not generate ASF License warnings. |
| | | 224m 59s | | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3423/3/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hadoop/pull/3423 |
| Optional Tests | dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell |
| uname | Linux fbe9089dd010 4.15.0-147-generic #151-Ubuntu SMP Fri Jun 18 19:21:19 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/bin/hadoop.sh |
| git revision | trunk / d82e53f9626373492899ce413ed64cb2ad9bb1a8 |
| Default Java | Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 |
| Multi-JDK versions | /usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 |
| Test Results | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3423/3/testReport/ |
| Max. process+thread count | 1252 (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-3423/3/console |
| versions | git=2.25.1 maven=3.6.3 spotbugs=4.2.2 |
| Powered by | Apache Yetus 0.14.0-SNAPSHOT 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] bharatviswa504 commented on pull request #3423: HADOOP-17905. Modify Text.ensureCapacity() to efficiently max out the…
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on pull request #3423:
URL: https://github.com/apache/hadoop/pull/3423#issuecomment-930643770
Thank You @pbacsko for the contribution and @goiri for the careful reviews.
--
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] pbacsko commented on a change in pull request #3423: HADOOP-17905. Modify Text.ensureCapacity() to efficiently max out the…
Posted by GitBox <gi...@apache.org>.
pbacsko commented on a change in pull request #3423:
URL: https://github.com/apache/hadoop/pull/3423#discussion_r706610924
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/Text.java
##########
@@ -301,9 +305,18 @@ public void clear() {
*/
private boolean ensureCapacity(final int capacity) {
if (bytes.length < capacity) {
+ // use long to allow overflow
Review comment:
Optionally, we can also check here if capacity > MAX_ARRAY_SIZE and fail gracefully with a good error message. But it's not strictly necessary, System.arrayCopy() would fail later.
--
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