Variable size ByteBuffer implementation #99

gkumar7 · 2017-02-16T02:49:13Z

This patch includes an implementation of a variable-size BinaryHolder to abstract away size calculation for a ByteBuffer. The BinaryHolder is flexible enough to be used as a replacement for the Binary format. For more information, see #95.

@bensigelman, @yurishkuro, @wu-sheng, @mabn, @michaelsembwever could you please review?

bhs · 2017-03-09T05:54:16Z

@gkumar7 sorry for the insane delay, I am looking at pending issues/PRs briefly. This branch has a merge conflict right now which is problematic... can you address?

gkumar7 · 2017-03-09T16:03:00Z

@bhs Thanks for the response! Seems like there were a few conflicting changes while this review was out, fixed now.

bhs

@yurishkuro WDYT?

bhs · 2017-03-10T06:14:53Z

opentracing-api/src/main/java/io/opentracing/propagation/Format.java

@@ -79,6 +79,16 @@ private Builtin(String name) {
        public final static Format<ByteBuffer> BINARY = new Builtin<ByteBuffer>("BINARY");

        /**
+         * The BINARY_HOLDER format allows for variable binary encoding of SpanContext state for Tracer.inject and


nit: "... variable-length binary ..."

bhs · 2017-03-10T06:16:09Z

opentracing-api/src/main/java/io/opentracing/propagation/BinaryHolder.java

+
+    /**
+     * If no initial ByteBuffer was allocated for the BinaryHolder, the payload becomes the wrapped ByteBuffer.
+     * Otherwise, the payload is placed in existent ByteBuffer.


nit: "existing"

bhs · 2017-03-10T06:16:42Z

opentracing-api/src/main/java/io/opentracing/propagation/BinaryHolder.java

+     *
+     * @param payload a ByteBuffer.
+     */
+    public void addPayload(ByteBuffer payload) {


why addPayload and getCarrier? shouldn't the names be symmetric?

Good point, fixed.

yurishkuro · 2017-03-10T18:33:48Z

@bhs I don't have strong feeling about it. I never particularly liked the ByteBuffer-based API, I would rather use java.io streams.

gkumar7 · 2017-03-17T01:11:59Z

java.io.streams would be an interesting approach, although a bit tricky as there are both input and output streams. I do not think one Format specification will be able to handle both cases

yurishkuro · 2017-03-17T03:10:35Z

bit tricky as there are both input and output streams. I do not think one Format specification will be able to handle both cases

@gkumar7 that's why my original proposal for Format was not Format<C> but Format<R, W>, but it was shot down as "confusing" ¯_(ツ)_/¯

gkumar7 · 2017-03-17T14:24:08Z

Ah, that makes sense. @bhs could we merge these changes for now so people have a workaround to the ByteBuffer format and then we can discuss more about the Format specification in a separate issue?

yurishkuro · 2017-03-17T14:56:40Z

could we merge these changes for now

@gkumar7 I'd want to be careful, because merging this has implications, it becomes a requirement for all existing tracers to support this new carrier.

gkumar7 · 2017-03-22T13:53:06Z

Yes, very true, but I would argue that tracers should support a BinaryHolder to provide users of the library an alternative to the fundamental limitation of a ByteBuffer.

The ByteBuffer format is there, but many java tracing libraries do not formally provide an example usage. Anyone using the ByteBuffer will hit a roadblock when having to specify the size of the ByteBuffer before serialization occurs.

bhs · 2017-04-01T21:48:11Z

@gkumar7 FYI, I have been thinking about this some more, apologies for the insanely high latency.

I would like to merge this change, but not until we're close to cutting a new major release for OT-Java... maybe we can get rid of the other binary format altogether at that point (since we're introducing breaking changes anyway).

I created a milestone for a 1.0 release, btw: https://github.com/opentracing/opentracing-java/milestone/1

gkumar7 · 2017-04-02T16:06:53Z

@bhs Sounds good, I will keep this pr open until then. Thanks!

tedsuo · 2017-11-09T19:35:24Z

@gkumar7 @yurishkuro I propose this change should get rolled into 0.31. We'll need to update this PR to point at the opentracing:v0.31.0 branch.

I'm wondering if anyone supports the current BINARY carrier type in java, and whether we can change that type rather than add BINARY_HOLDER.

gkumar7 · 2017-11-16T03:33:18Z

+1 to update BINARY type, our team has not been using it.

malafeev · 2017-11-29T05:27:40Z

+1
I need it to implement cassandra server side tracing. Payload is sent using ByteBuffer there.

carlosalberto · 2017-11-29T06:57:52Z

opentracing-impl/src/test/java/io/opentracing/impl/TestBinaryHolder.java

+
+    @Test
+    public void testBinaryHolderWithSufficientCapacity() {
+        System.out.println("binary holder with sufficient capacity");


When/if we get this PR merged, we should get rid of these println lines ;)

As you might have noticed this pull request was hanging around for about 9 months. The opentracing-java code has changed quite a bit since then. Initially, the structure of the test code in opentracing-impl (this module has been removed) had test classes with a println at the beginning of each method. Even I was not sure of the reason for this, but included similar lines in this pull request to remain consistent with the rest of the codebase.

carlosalberto · 2017-11-29T07:00:38Z

So my impression, after going through the comments and #95, is that people need some kind of binary format now. So what about:

Removing/deprecating the current BINARY format.
Adding this BinaryHolder format?
Later, as/when needed, add a BINARY_STREAM format?

That way, we could have people use their code using the required binary format, and later on offer to a stream-based binary format.

Opinions?

yurishkuro · 2017-11-29T14:12:09Z

@carlosalberto I'm not seeing the benefits of such approach, it just creates an API churn. The holder class is already new/breaking API, why not just define an equivalent of Go's io.Reader/io.Writer now and make it the only BINARY format? Those are very small interfaces. The holder class can be an adapter for them.

tedsuo · 2017-11-29T22:58:43Z

I agree, let's go with a streaming BINARY format. Since this will be a breaking change, let's get it in on v31.

carlosalberto · 2017-11-30T22:16:38Z

@gkumar7 @malafeev Hey, would you mind jumping into the discussion/review of #223 ? You may be interested as that's an effort to support BINARY through a stream-friendly interface (the PR itself inspired in this one ;) )

malafeev · 2017-12-01T01:49:28Z

@carlosalberto I added comment to #223 .
Without seeing how it would work in MockTracer inject/extract it's hard to say.

gkumar7 · 2017-12-01T03:26:12Z

@carlosalberto, thanks for creating a new pull request (although it would have been nice to make changes to the existing one). Regardless, I would agree with @malafeev that some example usage would be helpful.

gkumar7 · 2017-12-06T19:04:52Z

Closing as #223 is merged.

Variable size ByteBuffer implementation

a489dc7

gkumar7 changed the title ~~Variable size ByteBuffer implementation (Implementation of #95)~~ Variable size ByteBuffer implementation Feb 16, 2017

Fix merge-conflict in Format.java

91d7a74

bhs reviewed Mar 10, 2017

View reviewed changes

A few grammatical fixes

83b56b0

bhs mentioned this pull request Jun 17, 2017

Support tracing in Cassandra itself opentracing-contrib/java-cassandra-driver#1

Open

tedsuo mentioned this pull request Nov 9, 2017

0.31.0 Release Candidate #189

Closed

carlosalberto reviewed Nov 29, 2017

View reviewed changes

carlosalberto mentioned this pull request Nov 30, 2017

Change BINARY to be resizable and stream-oriented. #223

Merged

gkumar7 closed this Dec 6, 2017

gkumar7 deleted the variable-byte-buffer-holder branch December 6, 2017 19:05

carlosalberto mentioned this pull request May 18, 2018

Binary support design #253

Open

carlosalberto mentioned this pull request Jun 2, 2018

Simple layer on top of ByteBuffer for BINARY format. #276

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Variable size ByteBuffer implementation #99

Variable size ByteBuffer implementation #99

gkumar7 commented Feb 16, 2017 •

edited

Loading

bhs commented Mar 9, 2017

gkumar7 commented Mar 9, 2017

bhs left a comment

bhs Mar 10, 2017

bhs Mar 10, 2017

bhs Mar 10, 2017

gkumar7 Mar 10, 2017

yurishkuro commented Mar 10, 2017

gkumar7 commented Mar 17, 2017

yurishkuro commented Mar 17, 2017

gkumar7 commented Mar 17, 2017

yurishkuro commented Mar 17, 2017

gkumar7 commented Mar 22, 2017

bhs commented Apr 1, 2017 •

edited

Loading

gkumar7 commented Apr 2, 2017

tedsuo commented Nov 9, 2017

gkumar7 commented Nov 16, 2017

malafeev commented Nov 29, 2017

carlosalberto Nov 29, 2017

gkumar7 Dec 1, 2017 •

edited

Loading

carlosalberto commented Nov 29, 2017

yurishkuro commented Nov 29, 2017

tedsuo commented Nov 29, 2017

carlosalberto commented Nov 30, 2017

malafeev commented Dec 1, 2017

gkumar7 commented Dec 1, 2017

gkumar7 commented Dec 6, 2017

Variable size ByteBuffer implementation #99

Variable size ByteBuffer implementation #99

Conversation

gkumar7 commented Feb 16, 2017 • edited Loading

bhs commented Mar 9, 2017

gkumar7 commented Mar 9, 2017

bhs left a comment

Choose a reason for hiding this comment

bhs Mar 10, 2017

Choose a reason for hiding this comment

bhs Mar 10, 2017

Choose a reason for hiding this comment

bhs Mar 10, 2017

Choose a reason for hiding this comment

gkumar7 Mar 10, 2017

Choose a reason for hiding this comment

yurishkuro commented Mar 10, 2017

gkumar7 commented Mar 17, 2017

yurishkuro commented Mar 17, 2017

gkumar7 commented Mar 17, 2017

yurishkuro commented Mar 17, 2017

gkumar7 commented Mar 22, 2017

bhs commented Apr 1, 2017 • edited Loading

gkumar7 commented Apr 2, 2017

tedsuo commented Nov 9, 2017

gkumar7 commented Nov 16, 2017

malafeev commented Nov 29, 2017

carlosalberto Nov 29, 2017

Choose a reason for hiding this comment

gkumar7 Dec 1, 2017 • edited Loading

Choose a reason for hiding this comment

carlosalberto commented Nov 29, 2017

yurishkuro commented Nov 29, 2017

tedsuo commented Nov 29, 2017

carlosalberto commented Nov 30, 2017

malafeev commented Dec 1, 2017

gkumar7 commented Dec 1, 2017

gkumar7 commented Dec 6, 2017

gkumar7 commented Feb 16, 2017 •

edited

Loading

bhs commented Apr 1, 2017 •

edited

Loading

gkumar7 Dec 1, 2017 •

edited

Loading