You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@spark.apache.org by Sean Owen <so...@cloudera.com> on 2014/12/09 22:53:08 UTC
Is this a little bug in BlockTransferMessage ?
https://github.com/apache/spark/blob/master/network/shuffle/src/main/java/org/apache/spark/network/shuffle/protocol/BlockTransferMessage.java#L70
public byte[] toByteArray() {
ByteBuf buf = Unpooled.buffer(encodedLength());
buf.writeByte(type().id);
encode(buf);
assert buf.writableBytes() == 0 : "Writable bytes remain: " +
buf.writableBytes();
return buf.array();
}
Running the Java tests at last might have turned up a little bug here,
but wanted to check. This makes a buffer to hold enough bytes to
encode the message. But it writes 1 byte, plus the message. This makes
the buffer expand, and then does have nonzero capacity afterwards, so
the assert fails.
So just needs a "+ 1" in the size?
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@spark.apache.org
For additional commands, e-mail: dev-help@spark.apache.org
Re: Is this a little bug in BlockTransferMessage ?
Posted by Sean Owen <so...@cloudera.com>.
Oops, yes I see Java tests run with SBT now. You're right, it must be
because of the assertion. I can try to add '-ea' to the SBT build as a
closely-related change for SPARK-4159.
FWIW this error is the only one I saw once the Maven tests ran the Java tests.
On Wed, Dec 10, 2014 at 6:07 AM, Patrick Wendell <pw...@gmail.com> wrote:
> Hey Nick,
>
> Thanks for bringing this up. I believe these Java tests are running in
> the sbt build right now, the issue is that this particular bug was
> flagged by the triggering of a runtime Java "assert" (not a normal
> Junit test assertion) and those are not enabled in our sbt tests. It
> would be good to fix it so that assertions run when we do the sbt
> tests, for some reason I think the sbt tests disable them by default.
>
> I think the original issue is fixed now (that Sean found and
> reported). It would be good to get assertions running in our tests,
> but I'm not sure I'd block the release on it. The normal JUnit
> assertions are running correctly.
>
> - Patrick
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@spark.apache.org
For additional commands, e-mail: dev-help@spark.apache.org
Re: Is this a little bug in BlockTransferMessage ?
Posted by Patrick Wendell <pw...@gmail.com>.
Hey Nick,
Thanks for bringing this up. I believe these Java tests are running in
the sbt build right now, the issue is that this particular bug was
flagged by the triggering of a runtime Java "assert" (not a normal
Junit test assertion) and those are not enabled in our sbt tests. It
would be good to fix it so that assertions run when we do the sbt
tests, for some reason I think the sbt tests disable them by default.
I think the original issue is fixed now (that Sean found and
reported). It would be good to get assertions running in our tests,
but I'm not sure I'd block the release on it. The normal JUnit
assertions are running correctly.
- Patrick
On Tue, Dec 9, 2014 at 3:35 PM, Nicholas Chammas
<ni...@gmail.com> wrote:
> OK. That's concerning. Hopefully that's the only bug we'll dig up once we
> run all the Java tests but who knows.
>
> Patrick,
>
> Shouldn't this be a release blocking bug for 1.2 (mostly just because it
> has already been covered by a unit test)? Well, that, as well as any other
> bugs that come up as we run these Java tests.
>
> Nick
>
> On Tue Dec 09 2014 at 6:32:53 PM Sean Owen <so...@cloudera.com> wrote:
>
>> I'm not so sure about SBT, but I'm looking at the output now and do
>> not see things like JavaAPISuite being run. I see them compiled. That
>> I'm not as sure how to fix. I think I have a solution for Maven on
>> SPARK-4159.
>>
>> On Tue, Dec 9, 2014 at 11:30 PM, Nicholas Chammas
>> <ni...@gmail.com> wrote:
>> > So all this time the tests that Jenkins has been running via Jenkins and
>> SBT
>> > + ScalaTest... those haven't been running any of the Java unit tests?
>> >
>> > SPARK-4159 only mentions Maven as a problem, but I'm wondering how these
>> > tests got through Jenkins OK.
>> >
>> > On Tue Dec 09 2014 at 5:34:22 PM Sean Owen <so...@cloudera.com> wrote:
>> >>
>> >> Yep, will do. The test does catch it -- it's just not being executed.
>> >> I think I have a reasonable start on re-enabling surefire + Java tests
>> >> for SPARK-4159.
>> >>
>> >> On Tue, Dec 9, 2014 at 10:30 PM, Aaron Davidson <aa...@databricks.com>
>> >> wrote:
>> >> > Oops, that does look like a bug. Strange that the
>> >> > BlockTransferMessageSuite
>> >> > did not catch this. "+1" sounds like the right solution, would you be
>> >> > able
>> >> > to submit a PR?
>> >> >
>> >> > On Tue, Dec 9, 2014 at 1:53 PM, Sean Owen <so...@cloudera.com> wrote:
>> >> >>
>> >> >>
>> >> >>
>> >> >> https://github.com/apache/spark/blob/master/network/
>> shuffle/src/main/java/org/apache/spark/network/shuffle/
>> protocol/BlockTransferMessage.java#L70
>> >> >>
>> >> >> public byte[] toByteArray() {
>> >> >> ByteBuf buf = Unpooled.buffer(encodedLength());
>> >> >> buf.writeByte(type().id);
>> >> >> encode(buf);
>> >> >> assert buf.writableBytes() == 0 : "Writable bytes remain: " +
>> >> >> buf.writableBytes();
>> >> >> return buf.array();
>> >> >> }
>> >> >>
>> >> >> Running the Java tests at last might have turned up a little bug
>> here,
>> >> >> but wanted to check. This makes a buffer to hold enough bytes to
>> >> >> encode the message. But it writes 1 byte, plus the message. This
>> makes
>> >> >> the buffer expand, and then does have nonzero capacity afterwards, so
>> >> >> the assert fails.
>> >> >>
>> >> >> So just needs a "+ 1" in the size?
>> >> >
>> >> >
>> >>
>> >> ---------------------------------------------------------------------
>> >> To unsubscribe, e-mail: dev-unsubscribe@spark.apache.org
>> >> For additional commands, e-mail: dev-help@spark.apache.org
>> >>
>> >
>>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@spark.apache.org
For additional commands, e-mail: dev-help@spark.apache.org
Re: Is this a little bug in BlockTransferMessage ?
Posted by Nicholas Chammas <ni...@gmail.com>.
OK. That's concerning. Hopefully that's the only bug we'll dig up once we
run all the Java tests but who knows.
Patrick,
Shouldn't this be a release blocking bug for 1.2 (mostly just because it
has already been covered by a unit test)? Well, that, as well as any other
bugs that come up as we run these Java tests.
Nick
On Tue Dec 09 2014 at 6:32:53 PM Sean Owen <so...@cloudera.com> wrote:
> I'm not so sure about SBT, but I'm looking at the output now and do
> not see things like JavaAPISuite being run. I see them compiled. That
> I'm not as sure how to fix. I think I have a solution for Maven on
> SPARK-4159.
>
> On Tue, Dec 9, 2014 at 11:30 PM, Nicholas Chammas
> <ni...@gmail.com> wrote:
> > So all this time the tests that Jenkins has been running via Jenkins and
> SBT
> > + ScalaTest... those haven't been running any of the Java unit tests?
> >
> > SPARK-4159 only mentions Maven as a problem, but I'm wondering how these
> > tests got through Jenkins OK.
> >
> > On Tue Dec 09 2014 at 5:34:22 PM Sean Owen <so...@cloudera.com> wrote:
> >>
> >> Yep, will do. The test does catch it -- it's just not being executed.
> >> I think I have a reasonable start on re-enabling surefire + Java tests
> >> for SPARK-4159.
> >>
> >> On Tue, Dec 9, 2014 at 10:30 PM, Aaron Davidson <aa...@databricks.com>
> >> wrote:
> >> > Oops, that does look like a bug. Strange that the
> >> > BlockTransferMessageSuite
> >> > did not catch this. "+1" sounds like the right solution, would you be
> >> > able
> >> > to submit a PR?
> >> >
> >> > On Tue, Dec 9, 2014 at 1:53 PM, Sean Owen <so...@cloudera.com> wrote:
> >> >>
> >> >>
> >> >>
> >> >> https://github.com/apache/spark/blob/master/network/
> shuffle/src/main/java/org/apache/spark/network/shuffle/
> protocol/BlockTransferMessage.java#L70
> >> >>
> >> >> public byte[] toByteArray() {
> >> >> ByteBuf buf = Unpooled.buffer(encodedLength());
> >> >> buf.writeByte(type().id);
> >> >> encode(buf);
> >> >> assert buf.writableBytes() == 0 : "Writable bytes remain: " +
> >> >> buf.writableBytes();
> >> >> return buf.array();
> >> >> }
> >> >>
> >> >> Running the Java tests at last might have turned up a little bug
> here,
> >> >> but wanted to check. This makes a buffer to hold enough bytes to
> >> >> encode the message. But it writes 1 byte, plus the message. This
> makes
> >> >> the buffer expand, and then does have nonzero capacity afterwards, so
> >> >> the assert fails.
> >> >>
> >> >> So just needs a "+ 1" in the size?
> >> >
> >> >
> >>
> >> ---------------------------------------------------------------------
> >> To unsubscribe, e-mail: dev-unsubscribe@spark.apache.org
> >> For additional commands, e-mail: dev-help@spark.apache.org
> >>
> >
>
Re: Is this a little bug in BlockTransferMessage ?
Posted by Sean Owen <so...@cloudera.com>.
I'm not so sure about SBT, but I'm looking at the output now and do
not see things like JavaAPISuite being run. I see them compiled. That
I'm not as sure how to fix. I think I have a solution for Maven on
SPARK-4159.
On Tue, Dec 9, 2014 at 11:30 PM, Nicholas Chammas
<ni...@gmail.com> wrote:
> So all this time the tests that Jenkins has been running via Jenkins and SBT
> + ScalaTest... those haven't been running any of the Java unit tests?
>
> SPARK-4159 only mentions Maven as a problem, but I'm wondering how these
> tests got through Jenkins OK.
>
> On Tue Dec 09 2014 at 5:34:22 PM Sean Owen <so...@cloudera.com> wrote:
>>
>> Yep, will do. The test does catch it -- it's just not being executed.
>> I think I have a reasonable start on re-enabling surefire + Java tests
>> for SPARK-4159.
>>
>> On Tue, Dec 9, 2014 at 10:30 PM, Aaron Davidson <aa...@databricks.com>
>> wrote:
>> > Oops, that does look like a bug. Strange that the
>> > BlockTransferMessageSuite
>> > did not catch this. "+1" sounds like the right solution, would you be
>> > able
>> > to submit a PR?
>> >
>> > On Tue, Dec 9, 2014 at 1:53 PM, Sean Owen <so...@cloudera.com> wrote:
>> >>
>> >>
>> >>
>> >> https://github.com/apache/spark/blob/master/network/shuffle/src/main/java/org/apache/spark/network/shuffle/protocol/BlockTransferMessage.java#L70
>> >>
>> >> public byte[] toByteArray() {
>> >> ByteBuf buf = Unpooled.buffer(encodedLength());
>> >> buf.writeByte(type().id);
>> >> encode(buf);
>> >> assert buf.writableBytes() == 0 : "Writable bytes remain: " +
>> >> buf.writableBytes();
>> >> return buf.array();
>> >> }
>> >>
>> >> Running the Java tests at last might have turned up a little bug here,
>> >> but wanted to check. This makes a buffer to hold enough bytes to
>> >> encode the message. But it writes 1 byte, plus the message. This makes
>> >> the buffer expand, and then does have nonzero capacity afterwards, so
>> >> the assert fails.
>> >>
>> >> So just needs a "+ 1" in the size?
>> >
>> >
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@spark.apache.org
>> For additional commands, e-mail: dev-help@spark.apache.org
>>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@spark.apache.org
For additional commands, e-mail: dev-help@spark.apache.org
Re: Is this a little bug in BlockTransferMessage ?
Posted by Nicholas Chammas <ni...@gmail.com>.
So all this time the tests that Jenkins has been running via Jenkins and
SBT + ScalaTest... those haven't been running any of the Java unit tests?
SPARK-4159 <https://issues.apache.org/jira/browse/SPARK-4159> only mentions
Maven as a problem, but I'm wondering how these tests got through Jenkins
OK.
On Tue Dec 09 2014 at 5:34:22 PM Sean Owen <so...@cloudera.com> wrote:
> Yep, will do. The test does catch it -- it's just not being executed.
> I think I have a reasonable start on re-enabling surefire + Java tests
> for SPARK-4159.
>
> On Tue, Dec 9, 2014 at 10:30 PM, Aaron Davidson <aa...@databricks.com>
> wrote:
> > Oops, that does look like a bug. Strange that the
> BlockTransferMessageSuite
> > did not catch this. "+1" sounds like the right solution, would you be
> able
> > to submit a PR?
> >
> > On Tue, Dec 9, 2014 at 1:53 PM, Sean Owen <so...@cloudera.com> wrote:
> >>
> >>
> >> https://github.com/apache/spark/blob/master/network/
> shuffle/src/main/java/org/apache/spark/network/shuffle/
> protocol/BlockTransferMessage.java#L70
> >>
> >> public byte[] toByteArray() {
> >> ByteBuf buf = Unpooled.buffer(encodedLength());
> >> buf.writeByte(type().id);
> >> encode(buf);
> >> assert buf.writableBytes() == 0 : "Writable bytes remain: " +
> >> buf.writableBytes();
> >> return buf.array();
> >> }
> >>
> >> Running the Java tests at last might have turned up a little bug here,
> >> but wanted to check. This makes a buffer to hold enough bytes to
> >> encode the message. But it writes 1 byte, plus the message. This makes
> >> the buffer expand, and then does have nonzero capacity afterwards, so
> >> the assert fails.
> >>
> >> So just needs a "+ 1" in the size?
> >
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@spark.apache.org
> For additional commands, e-mail: dev-help@spark.apache.org
>
>
Re: Is this a little bug in BlockTransferMessage ?
Posted by Sean Owen <so...@cloudera.com>.
Yep, will do. The test does catch it -- it's just not being executed.
I think I have a reasonable start on re-enabling surefire + Java tests
for SPARK-4159.
On Tue, Dec 9, 2014 at 10:30 PM, Aaron Davidson <aa...@databricks.com> wrote:
> Oops, that does look like a bug. Strange that the BlockTransferMessageSuite
> did not catch this. "+1" sounds like the right solution, would you be able
> to submit a PR?
>
> On Tue, Dec 9, 2014 at 1:53 PM, Sean Owen <so...@cloudera.com> wrote:
>>
>>
>> https://github.com/apache/spark/blob/master/network/shuffle/src/main/java/org/apache/spark/network/shuffle/protocol/BlockTransferMessage.java#L70
>>
>> public byte[] toByteArray() {
>> ByteBuf buf = Unpooled.buffer(encodedLength());
>> buf.writeByte(type().id);
>> encode(buf);
>> assert buf.writableBytes() == 0 : "Writable bytes remain: " +
>> buf.writableBytes();
>> return buf.array();
>> }
>>
>> Running the Java tests at last might have turned up a little bug here,
>> but wanted to check. This makes a buffer to hold enough bytes to
>> encode the message. But it writes 1 byte, plus the message. This makes
>> the buffer expand, and then does have nonzero capacity afterwards, so
>> the assert fails.
>>
>> So just needs a "+ 1" in the size?
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@spark.apache.org
For additional commands, e-mail: dev-help@spark.apache.org