Skip to content

[SPARK-15601][CORE] CircularBuffer's toString() to print only the contents written if buffer isn't full #13351

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
tejasapatil wants to merge 4 commits into from
Closed

[SPARK-15601][CORE] CircularBuffer's toString() to print only the contents written if buffer isn't full #13351

tejasapatil wants to merge 4 commits into from

Conversation

@tejasapatil
Copy link
Contributor

@tejasapatil tejasapatil commented May 27, 2016 •
edited
Loading

What changes were proposed in this pull request?

  1. The class allocated 4x space than needed as it was using Int to store the Byte values
  2. If CircularBuffer isn't full, currently toString() will print some garbage chars along with the content written as is tries to print the entire array allocated for the buffer. The fix is to keep track of buffer getting full and don't print the tail of the buffer if it isn't full (suggestion by @sameeragarwal over [SPARK-14400] [SQL] ScriptTransformation does not fail the job for bad user command #12194 (comment))
  3. Simplified toString()

How was this patch tested?

Added new test case

@tejasapatil tejasapatil mentioned this pull request May 27, 2016
Closed
@tejasapatil
Copy link
Contributor Author

tejasapatil commented May 27, 2016

ok to test

@SparkQA
Copy link

SparkQA commented May 27, 2016

Test build #59470 has finished for PR 13351 at commit 24e50bb.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

srowen
srowen reviewed May 27, 2016
View reviewed changes
core/src/main/scala/org/apache/spark/util/Utils.scala Outdated
private[spark] class CircularBuffer(sizeInBytes: Int = 10240) extends java.io.OutputStream {
var pos: Int = 0
var isBufferFull = false
var buffer = new Array[Int](sizeInBytes)
Copy link
Member

@srowen srowen May 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we have more problems with this class. This should be val buffer = new Array[Byte](sizeInBytes). It's 4x larger than it needs to be.

Copy link
Contributor Author

@tejasapatil tejasapatil May 30, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch !! Its definitely worth fixing this.

@srowen
Copy link
Member

srowen commented May 27, 2016

CC @chenghao-intel

@SparkQA
Copy link

SparkQA commented May 30, 2016

Test build #59602 has finished for PR 13351 at commit 3a1e7ff.

  • This patch fails Scala style tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented May 30, 2016

Test build #59603 has finished for PR 13351 at commit 76b6e3c.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

srowen
srowen reviewed May 30, 2016
View reviewed changes
core/src/main/scala/org/apache/spark/util/Utils.scala Outdated
var pos: Int = 0
var buffer = new Array[Int](sizeInBytes)
var isBufferFull = false
var buffer = new Array[Byte](sizeInBytes)
Copy link
Member

@srowen srowen May 30, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be val?

Copy link
Contributor Author

@tejasapatil tejasapatil May 30, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed

@srowen
Copy link
Member

srowen commented May 31, 2016

Yeah that LGTM

@SparkQA
Copy link

SparkQA commented May 31, 2016

Test build #59622 has finished for PR 13351 at commit a0ae62e.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@tejasapatil
Copy link
Contributor Author

tejasapatil commented May 31, 2016

ok to test.

I ran sbt core/test:test locally and it worked. Not sure what failed over Jenkins because it says [info] Passed: Total 1832, Failed 0, Errors 0, Passed 1832, Ignored 591

@tejasapatil
Copy link
Contributor Author

tejasapatil commented May 31, 2016

Jenkins, retest this please

@SparkQA
Copy link

SparkQA commented May 31, 2016

Test build #59628 has finished for PR 13351 at commit a0ae62e.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@sameeragarwal
Copy link
Member

sameeragarwal commented May 31, 2016

Thanks Tejas!

asfgit pushed a commit that referenced this pull request Jun 1, 2016
@tejasapatil @srowen
[SPARK-15601][CORE] CircularBuffer's toString() to print only the con…
d34c0fc 
…tents written if buffer isn't full

## What changes were proposed in this pull request?

1. The class allocated 4x space than needed as it was using `Int` to store the `Byte` values

2. If CircularBuffer isn't full, currently toString() will print some garbage chars along with the content written as is tries to print the entire array allocated for the buffer. The fix is to keep track of buffer getting full and don't print the tail of the buffer if it isn't full (suggestion by sameeragarwal over #12194 (comment))

3. Simplified `toString()`

## How was this patch tested?

Added new test case

Author: Tejas Patil <tejasp@fb.com>

Closes #13351 from tejasapatil/circular_buffer.

(cherry picked from commit ac38bdc)
Signed-off-by: Sean Owen <sowen@cloudera.com>
@asfgit asfgit closed this in ac38bdc Jun 1, 2016
asfgit pushed a commit that referenced this pull request Jun 1, 2016
@tejasapatil @srowen
[SPARK-15601][CORE] CircularBuffer's toString() to print only the con…
714f4d7 
…tents written if buffer isn't full

1. The class allocated 4x space than needed as it was using `Int` to store the `Byte` values

2. If CircularBuffer isn't full, currently toString() will print some garbage chars along with the content written as is tries to print the entire array allocated for the buffer. The fix is to keep track of buffer getting full and don't print the tail of the buffer if it isn't full (suggestion by sameeragarwal over #12194 (comment))

3. Simplified `toString()`

Added new test case

Author: Tejas Patil <tejasp@fb.com>

Closes #13351 from tejasapatil/circular_buffer.

(cherry picked from commit ac38bdc)
Signed-off-by: Sean Owen <sowen@cloudera.com>
@srowen
Copy link
Member

srowen commented Jun 1, 2016

Merged to master/2.0/1.6

zzcclp pushed a commit to zzcclp/spark that referenced this pull request Jun 1, 2016
@tejasapatil @zzcclp
[SPARK-15601][CORE] CircularBuffer's toString() to print only the con…
23395c8 
…tents written if buffer isn't full

1. The class allocated 4x space than needed as it was using `Int` to store the `Byte` values

2. If CircularBuffer isn't full, currently toString() will print some garbage chars along with the content written as is tries to print the entire array allocated for the buffer. The fix is to keep track of buffer getting full and don't print the tail of the buffer if it isn't full (suggestion by sameeragarwal over apache#12194 (comment))

3. Simplified `toString()`

Added new test case

Author: Tejas Patil <tejasp@fb.com>

Closes apache#13351 from tejasapatil/circular_buffer.

(cherry picked from commit ac38bdc)
Signed-off-by: Sean Owen <sowen@cloudera.com>
(cherry picked from commit 714f4d7)
@tejasapatil tejasapatil deleted the circular_buffer branch July 6, 2016 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@tejasapatil @SparkQA @srowen @sameeragarwal