Using new windowTimeout operator with backpressure support

[anuchandy]

No description provided.

[anuchandy]

I was trying to see if the direct "reference" (i.e. non-reflective reference) to the new API here is safe? so far, my understanding is, the module gets compiled only in SDK repro and always with the reactor-core3.4.19 or above (i.e. both the new and old methods are available at compile time).

Anyone else will be using the published compiled jar (e.g., from maven), and "invocation" of one of the method overloads is protected by its discoverability at runtime, i.e., code won't try to load the new API with the older reactor-core.

But want to hear from others who have more expertise in doing this.

[anuchandy]

I found a somewhat similar direct reference example here https://github.com/netty/netty/blob/4.1/codec/src/main/java/io/netty/handler/codec/compression/JdkZlibEncoder.java#L334-L336

private void deflate(ByteBuf out) {
    if (PlatformDependent.javaVersion() < 7) {
        deflateJdk6(out);
    }
    int numBytes;
    do {
       ...
        numBytes = deflater.deflate(
                out.array(), out.arrayOffset() + writerIndex, 
                out.writableBytes(), Deflater.SYNC_FLUSH);
       ...
    } while (numBytes > 0);
}

The Deflater.SYNC_FLUSH is directly referenced; the enum value "SYNC_FLUSH" available only from Java7, and runtime access to the value is protected by a similar runtime check.

[liukun-msft]

I have a little bit concern for the checking. Because we may already or in the future use other new APIs in reactor-core3.4.19+, I am wondering if we need always add this checking for that code. If this is only temporary, I am ok for it.

And after users upgrade the SB version, are they better to upgrade the reactor-core version if they are using the old reactor-core and loading at runtime? we can mention that information in README.md. I am curious about the background of why they use the old reactor-core and load at runtime.

[ZejiaJiang]

Agree with Kun about the checking.

There is no background that users use old reactor-core I assume. If users don't need new feature for reactor-core or there is no bug in the past days, they may not upgrade it. So mention this information in README.md is enough.

[kasobol-msft]

@anuchandy Looks like we need ReactorShim. Like we did for jackson
@lmolkova might help here.

If we pursue this way, we should also make sure this work item happens .

[anuchandy]

Thanks Kamil, Kun & Zeija for the feedback.

Let me use ObjectMapperShim as a reference implementation to understand the design-pattern we established for cross-version handling. I'll follow up with Ludmila then.

[anuchandy]

I was going over JacksonShim and ObjectMapperFactory; I feel like introducing ReactorShim in the azure-core for this case might be over-engineering.

We're not changing the reactor-core minor or major version. Like other patch versions, there are no breaking changes in the "existing" operators. The case here is that only Event Hubs SDK wants to use a "new" window operator overload from the latest patch, but if the user override the patch, it'll continue to use the "existing" window operator.

Today, if a user overrides with a different patch (3.4.patch) version, they won't face any linkage/method-not-found error; we continue to give the same guarantee.

So I feel we could keep it simple and limit to Event Hubs impl, Like cosmos SDK applying warmup for reactor netty Client. At least that's what I'm thinking for the EH beta, which a customer is waiting to unblock their development.

[kasobol-msft]

Sounds good, if EH is the only place that needs it let's keep it local.
However, I'd suggest to create ReactorShim in the EH *.implementation.* package so that we prepare ourselves for the eventual move to azure-core with this when we get more of this or demand across SDKs

[anuchandy]

Cool, Yes!. How is this API-shape look like?

[kasobol-msft]

nice!

[anuchandy]

Closing this, opening a new PR with clean commits to keep the beta release branch clean while crew works on other beta feature