You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@pekko.apache.org by "pjfanning (via GitHub)" <gi...@apache.org> on 2024/01/30 15:31:48 UTC
[PR] [DRAFT] add getInputStream to ByteString [incubator-pekko]
pjfanning opened a new pull request, #1085:
URL: https://github.com/apache/incubator-pekko/pull/1085
See https://github.com/apache/incubator-pekko/issues/995
This needs tests but raising it for discussion.
This is probably not that useful for Pekko usage but general ByteString users might appreciate it.
This method gets an InputStream without cloning the array but is not unsafe like toArrayUnsafe which exposes the underlying array data in a way that it can be unsafely changed. The InputStream wraps this unsafe array in a way that doesn't allow it to be modified.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] add asInputStream to ByteString [incubator-pekko]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on PR #1085:
URL: https://github.com/apache/incubator-pekko/pull/1085#issuecomment-1922005968
It will defer the mapping if you don't read.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] add asInputStream to ByteString [incubator-pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #1085:
URL: https://github.com/apache/incubator-pekko/pull/1085#issuecomment-1922013082
> It will defer the mapping if you don't read.
it is being read - there is nothing gained here by thinking there is lazy eval anywhere here. SequenceInputStream takes an evaluated JavaEnumeration. For me, if we adjust this, we could write a custom function that maps a Vector to a JavaEnumeration without requiring the intermediate iterator.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] [DRAFT] add asInputStream to ByteString [incubator-pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #1085:
URL: https://github.com/apache/incubator-pekko/pull/1085#issuecomment-1920011615
this is still WIP
* more test coverage needed
* basic benchmarks added
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] add asInputStream to ByteString [incubator-pekko]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #1085:
URL: https://github.com/apache/incubator-pekko/pull/1085#discussion_r1475447021
##########
actor/src/main/scala-2.13/org/apache/pekko/util/ByteString.scala:
##########
@@ -578,6 +582,9 @@ object ByteString {
def asByteBuffers: scala.collection.immutable.Iterable[ByteBuffer] = bytestrings.map { _.asByteBuffer }
+ override def asInputStream: InputStream =
+ new SequenceInputStream(bytestrings.map(_.asInputStream).iterator.asJavaEnumeration)
Review Comment:
```suggestion
new SequenceInputStream(bytestrings.iterator.map(_.asInputStream).asJavaEnumeration)
```
##########
actor/src/main/scala-3/org/apache/pekko/util/ByteString.scala:
##########
@@ -579,6 +583,9 @@ object ByteString {
def asByteBuffers: scala.collection.immutable.Iterable[ByteBuffer] = bytestrings.map { _.asByteBuffer }
+ override def asInputStream: InputStream =
+ new SequenceInputStream(bytestrings.map(_.asInputStream).iterator.asJavaEnumeration)
Review Comment:
```suggestion
new SequenceInputStream(bytestrings.iterator.map(_.asInputStream).asJavaEnumeration)
```
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] add asInputStream to ByteString [incubator-pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning merged PR #1085:
URL: https://github.com/apache/incubator-pekko/pull/1085
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] add asInputStream to ByteString [incubator-pekko]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #1085:
URL: https://github.com/apache/incubator-pekko/pull/1085#discussion_r1475859925
##########
actor/src/main/scala-2.12/org/apache/pekko/util/ByteString.scala:
##########
@@ -566,6 +572,9 @@ object ByteString {
def asByteBuffers: scala.collection.immutable.Iterable[ByteBuffer] = bytestrings.map { _.asByteBuffer }
+ override def asInputStream: InputStream =
+ new SequenceInputStream(bytestrings.map(_.asInputStream).iterator.asJavaEnumeration)
Review Comment:
nit pick: it's more elegant to switch order,it's worth optimizing even income is insignificant！
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] add asInputStream to ByteString [incubator-pekko]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #1085:
URL: https://github.com/apache/incubator-pekko/pull/1085#discussion_r1474863377
##########
actor/src/main/scala-2.12/org/apache/pekko/util/ByteString.scala:
##########
@@ -566,6 +572,9 @@ object ByteString {
def asByteBuffers: scala.collection.immutable.Iterable[ByteBuffer] = bytestrings.map { _.asByteBuffer }
+ override def asInputStream: InputStream =
+ new SequenceInputStream(bytestrings.map(_.asInputStream).iterator.asJavaEnumeration)
Review Comment:
bytestrings.iterator.map(_.asInputString).asJavaEnumeration
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] add asInputStream to ByteString [incubator-pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #1085:
URL: https://github.com/apache/incubator-pekko/pull/1085#discussion_r1475972345
##########
actor/src/main/scala-2.12/org/apache/pekko/util/ByteString.scala:
##########
@@ -566,6 +572,9 @@ object ByteString {
def asByteBuffers: scala.collection.immutable.Iterable[ByteBuffer] = bytestrings.map { _.asByteBuffer }
+ override def asInputStream: InputStream =
+ new SequenceInputStream(bytestrings.map(_.asInputStream).iterator.asJavaEnumeration)
Review Comment:
turns out that @He-Pin 's suggestion does seem to run a little faster - so I've made the change
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] [DRAFT] add asInputStream to ByteString [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #1085:
URL: https://github.com/apache/incubator-pekko/pull/1085#discussion_r1472663426
##########
actor/src/main/scala-2.12/org/apache/pekko/util/ByteString.scala:
##########
@@ -827,6 +836,16 @@ sealed abstract class ByteString extends IndexedSeq[Byte] with IndexedSeqOptimiz
*/
def toArrayUnsafe(): Array[Byte] = toArray
+ /**
+ * Return the bytes in this ByteString as an InputStream.
+ *
+ * @return the bytes in this ByteString accessible as an InputStream
+ * @see [[asByteBuffer]]
+ * @since 1.1.0
+ */
+ def asInputStream: InputStream =
+ new SequenceInputStream(asByteBuffers.map(bb => new ByteBufferBackedInputStream(bb)).iterator.asJavaEnumeration)
Review Comment:
Agreed as it forces you to implement it for all subtypes which avoids leaving in a default sub-optimal (in terms of performance) implementation if a subtype happens to miss an implementation
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] add asInputStream to ByteString [incubator-pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #1085:
URL: https://github.com/apache/incubator-pekko/pull/1085#issuecomment-1920961420
I have a basic benchmark that shows that ByteString.asInputStream is faster than new ByteArrayInputStream(ByteString.toArray). The latter is the current safe way to create an InputStream for a ByteString.
I have also tested ByteArrayInputStream(ByteString.toArrayUnsafe) which is about as fast as ByteString.asInputStream for simple ByteStrings (single arrays) but is slow like ByteArrayInputStream(ByteString.toArray) when you have byte strings composed of many smaller byte strings.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] [DRAFT] add asInputStream to ByteString [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #1085:
URL: https://github.com/apache/incubator-pekko/pull/1085#issuecomment-1920015976
If it's not too pushy it would be good to get this out for M1, I suggested on mailing list we should do M1 around mid next week
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] add asInputStream to ByteString [incubator-pekko]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #1085:
URL: https://github.com/apache/incubator-pekko/pull/1085#discussion_r1475446900
##########
actor/src/main/scala-2.12/org/apache/pekko/util/ByteString.scala:
##########
@@ -566,6 +572,9 @@ object ByteString {
def asByteBuffers: scala.collection.immutable.Iterable[ByteBuffer] = bytestrings.map { _.asByteBuffer }
+ override def asInputStream: InputStream =
+ new SequenceInputStream(bytestrings.map(_.asInputStream).iterator.asJavaEnumeration)
Review Comment:
```suggestion
new SequenceInputStream(bytestrings.iterator.map(_.asInputStream).asJavaEnumeration)
```
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] [DRAFT] add asInputStream to ByteString [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #1085:
URL: https://github.com/apache/incubator-pekko/pull/1085#issuecomment-1919593717
> * mima is complaining
```
[error] pekko-actor: Failed binary compatibility check against org.apache.pekko:pekko-actor_2.12:1.0.0! Found 1 potential problems
[error] * abstract method asInputStream()java.io.InputStream in class org.apache.pekko.util.ByteString is present only in current version
[error] filter with: ProblemFilters.exclude[ReversedMissingMethodProblem]("org.apache.pekko.util.ByteString.asInputStream")
```
This is a forward compatibility error and is expected, i.e. MiMa complains when you add a new method to a public interface
> * should some benchmarks be written?
Yes they should
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] [DRAFT] add asInputStream to ByteString [incubator-pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #1085:
URL: https://github.com/apache/incubator-pekko/pull/1085#discussion_r1472645237
##########
actor/src/main/scala-2.12/org/apache/pekko/util/ByteString.scala:
##########
@@ -827,6 +827,15 @@ sealed abstract class ByteString extends IndexedSeq[Byte] with IndexedSeqOptimiz
*/
def toArrayUnsafe(): Array[Byte] = toArray
+ /**
+ * Return the bytes in this ByteString as an InputStream.
+ *
+ * @return the bytes in this ByteString accessible as an InputStream
+ * @see [[asByteBuffer]]
+ * @since 1.1.0
+ */
+ final def asInputStream: InputStream = new ByteArrayInputStream(toArrayUnsafe())
Review Comment:
good point - I have committed a change based on @jtjeferreira's suggestions
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] [DRAFT] add getInputStream to ByteString [incubator-pekko]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #1085:
URL: https://github.com/apache/incubator-pekko/pull/1085#discussion_r1471484775
##########
actor/src/main/scala-2.12/org/apache/pekko/util/ByteString.scala:
##########
@@ -827,6 +827,14 @@ sealed abstract class ByteString extends IndexedSeq[Byte] with IndexedSeqOptimiz
*/
def toArrayUnsafe(): Array[Byte] = toArray
+ /**
+ * Return the bytes in this ByteString as an InputStream.
+ *
+ * @return the bytes in this ByteString accessible as an InputStream
+ * @since 1.1.0
+ */
+ def getInputStream(): InputStream = new ByteArrayInputStream(toArrayUnsafe())
Review Comment:
For composiable Bytestring, I expect this call lazy.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] add asInputStream to ByteString [incubator-pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #1085:
URL: https://github.com/apache/incubator-pekko/pull/1085#discussion_r1475803347
##########
actor/src/main/scala-2.12/org/apache/pekko/util/ByteString.scala:
##########
@@ -566,6 +572,9 @@ object ByteString {
def asByteBuffers: scala.collection.immutable.Iterable[ByteBuffer] = bytestrings.map { _.asByteBuffer }
+ override def asInputStream: InputStream =
+ new SequenceInputStream(bytestrings.map(_.asInputStream).iterator.asJavaEnumeration)
Review Comment:
I have already stated that I don't think that this makes much difference
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] [DRAFT] add asInputStream to ByteString [incubator-pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #1085:
URL: https://github.com/apache/incubator-pekko/pull/1085#discussion_r1472720911
##########
actor/src/main/scala-2.12/org/apache/pekko/util/ByteString.scala:
##########
@@ -827,6 +836,16 @@ sealed abstract class ByteString extends IndexedSeq[Byte] with IndexedSeqOptimiz
*/
def toArrayUnsafe(): Array[Byte] = toArray
+ /**
+ * Return the bytes in this ByteString as an InputStream.
+ *
+ * @return the bytes in this ByteString accessible as an InputStream
+ * @see [[asByteBuffer]]
+ * @since 1.1.0
+ */
+ def asInputStream: InputStream =
+ new SequenceInputStream(asByteBuffers.map(bb => new ByteBufferBackedInputStream(bb)).iterator.asJavaEnumeration)
Review Comment:
removed the default impl
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] add asInputStream to ByteString [incubator-pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #1085:
URL: https://github.com/apache/incubator-pekko/pull/1085#issuecomment-1922001855
@He-Pin I think `bytestrings.iterator.map(_.asInputStream).asJavaEnumeration` will have the same overhead as `bytestrings.map(_.asInputStream).iterator.asJavaEnumeration`. I'd prefer not to make changes like this and then have to rerun all the benchmarks.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] add asInputStream to ByteString [incubator-pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #1085:
URL: https://github.com/apache/incubator-pekko/pull/1085#issuecomment-1921992636
> I just checked in Netty, there is a `public class ByteBufInputStream extends InputStream implements DataInput`, Do you think we can have a `class ByteStringInputString extends InputStream implements DataInput`?
We are dealing with wrapping byte arrays - the perfect InputStream for wrapping a byte array in ByteArrayInputStream.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] add asInputStream to ByteString [incubator-pekko]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on PR #1085:
URL: https://github.com/apache/incubator-pekko/pull/1085#issuecomment-1921944837
> > I just checked in Netty, there is a `public class ByteBufInputStream extends InputStream implements DataInput`, Do you think we can have a `class ByteStringInputString extends InputStream implements DataInput`?
>
> I checked that already. Unfortunately, Scala conversion to JavaEnumeration only seem to work on Scala Iterators and not on Scala Collections.
>
> With this change:
> ```
> [error] /Users/pj.fanning/code/incubator-pekko/actor/src/main/scala-2.13/org/apache/pekko/util/ByteString.scala:586:64: value asJavaEnumeration is not a member of scala.collection.immutable.Vector[java.io.InputStream]
> [error] new SequenceInputStream(bytestrings.map(_.asInputStream).asJavaEnumeration)
> ```
'bytestrings.iterator.map(_.asInputStream).asJavaEnumeration'
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] [DRAFT] add asInputStream to ByteString [incubator-pekko]
Posted by "jtjeferreira (via GitHub)" <gi...@apache.org>.
jtjeferreira commented on code in PR #1085:
URL: https://github.com/apache/incubator-pekko/pull/1085#discussion_r1472661239
##########
actor/src/main/scala-2.12/org/apache/pekko/util/ByteString.scala:
##########
@@ -827,6 +836,16 @@ sealed abstract class ByteString extends IndexedSeq[Byte] with IndexedSeqOptimiz
*/
def toArrayUnsafe(): Array[Byte] = toArray
+ /**
+ * Return the bytes in this ByteString as an InputStream.
+ *
+ * @return the bytes in this ByteString accessible as an InputStream
+ * @see [[asByteBuffer]]
+ * @since 1.1.0
+ */
+ def asInputStream: InputStream =
+ new SequenceInputStream(asByteBuffers.map(bb => new ByteBufferBackedInputStream(bb)).iterator.asJavaEnumeration)
Review Comment:
this should be an abstract method, right?
```suggestion
def asInputStream: InputStream
```
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] add asInputStream to ByteString [incubator-pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #1085:
URL: https://github.com/apache/incubator-pekko/pull/1085#issuecomment-1921905261
> I just checked in Netty, there is a `public class ByteBufInputStream extends InputStream implements DataInput`, Do you think we can have a `class ByteStringInputString extends InputStream implements DataInput`?
I checked that already. Unfortunately, Scala conversion to JavaEnumeration only seem to work on Scala Iterators and not on Scala Collections.
With this change:
```
[error] /Users/pj.fanning/code/incubator-pekko/actor/src/main/scala-2.13/org/apache/pekko/util/ByteString.scala:586:64: value asJavaEnumeration is not a member of scala.collection.immutable.Vector[java.io.InputStream]
[error] new SequenceInputStream(bytestrings.map(_.asInputStream).asJavaEnumeration)
```
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] add asInputStream to ByteString [incubator-pekko]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on PR #1085:
URL: https://github.com/apache/incubator-pekko/pull/1085#issuecomment-1921900505
I just checked in Netty, there is a `public class ByteBufInputStream extends InputStream implements DataInput`, Do you think we can have a `class ByteStringInputString extends InputStream implements DataInput`?
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] add asInputStream to ByteString [incubator-pekko]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #1085:
URL: https://github.com/apache/incubator-pekko/pull/1085#discussion_r1474974952
##########
actor/src/main/scala-2.12/org/apache/pekko/util/ByteString.scala:
##########
@@ -566,6 +572,9 @@ object ByteString {
def asByteBuffers: scala.collection.immutable.Iterable[ByteBuffer] = bytestrings.map { _.asByteBuffer }
+ override def asInputStream: InputStream =
+ new SequenceInputStream(bytestrings.map(_.asInputStream).iterator.asJavaEnumeration)
Review Comment:
@@pjfanning I will send a suggestion tomorrow about this line, currently on my phone.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] add asInputStream to ByteString [incubator-pekko]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #1085:
URL: https://github.com/apache/incubator-pekko/pull/1085#discussion_r1475833453
##########
actor/src/main/scala-2.12/org/apache/pekko/util/ByteString.scala:
##########
@@ -566,6 +572,9 @@ object ByteString {
def asByteBuffers: scala.collection.immutable.Iterable[ByteBuffer] = bytestrings.map { _.asByteBuffer }
+ override def asInputStream: InputStream =
+ new SequenceInputStream(bytestrings.map(_.asInputStream).iterator.asJavaEnumeration)
Review Comment:
Feel free, I have back hometown
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] [DRAFT] add getInputStream to ByteString [incubator-pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #1085:
URL: https://github.com/apache/incubator-pekko/pull/1085#discussion_r1471635913
##########
actor/src/main/scala-2.12/org/apache/pekko/util/ByteString.scala:
##########
@@ -827,6 +827,14 @@ sealed abstract class ByteString extends IndexedSeq[Byte] with IndexedSeqOptimiz
*/
def toArrayUnsafe(): Array[Byte] = toArray
+ /**
+ * Return the bytes in this ByteString as an InputStream.
+ *
+ * @return the bytes in this ByteString accessible as an InputStream
+ * @since 1.1.0
+ */
+ def getInputStream(): InputStream = new ByteArrayInputStream(toArrayUnsafe())
Review Comment:
I don't think it is appropriate to have this function return the same cached result. Input streams are stateful. Each consumer doing reads would be affected by any other consumer.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] add asInputStream to ByteString [incubator-pekko]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #1085:
URL: https://github.com/apache/incubator-pekko/pull/1085#discussion_r1475859925
##########
actor/src/main/scala-2.12/org/apache/pekko/util/ByteString.scala:
##########
@@ -566,6 +572,9 @@ object ByteString {
def asByteBuffers: scala.collection.immutable.Iterable[ByteBuffer] = bytestrings.map { _.asByteBuffer }
+ override def asInputStream: InputStream =
+ new SequenceInputStream(bytestrings.map(_.asInputStream).iterator.asJavaEnumeration)
Review Comment:
But still, it's worth optimizing even income is insignificant！
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] [DRAFT] add getInputStream to ByteString [incubator-pekko]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #1085:
URL: https://github.com/apache/incubator-pekko/pull/1085#discussion_r1471654511
##########
actor/src/main/scala-2.12/org/apache/pekko/util/ByteString.scala:
##########
@@ -827,6 +827,14 @@ sealed abstract class ByteString extends IndexedSeq[Byte] with IndexedSeqOptimiz
*/
def toArrayUnsafe(): Array[Byte] = toArray
+ /**
+ * Return the bytes in this ByteString as an InputStream.
+ *
+ * @return the bytes in this ByteString accessible as an InputStream
+ * @since 1.1.0
+ */
+ def getInputStream(): InputStream = new ByteArrayInputStream(toArrayUnsafe())
Review Comment:
How about name it `asInputString` or `toInputStream`?
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org