Where communities thrive


  • Join over 1.5M+ people
  • Join over 100K+ communities
  • Free without limits
  • Create your own community
People
Repo info
Activity
Moses Nakamura
@mosesn
possibly! I think we’re still a little nervous about adopting OT because it still feels like the early days. opentelemetry was announced less than two years ago, and we’re not as confident it will be a longterm standard as something like slf4j
I think a PR to change how the trace id is displayed would probably be accepted. making the type flexible would probably be pretty invasive though, so we’d want to discuss it before doing something like that
hllrsr
@hllrsr

Hi everyone!

I got a question about dependency versions and security issues. The latest release from finagle is using the zookeeper version "3.5.0-alpha" which uses a vulnerable version from io.netty/netty 3.x.
After reading some issues and PR's, I've saw this issue (twitter/finagle#665) mentioning some changes that make sure that finagle is not using netty on version 3.x anymore.
What I'm trying to figure out is if finagle is not using netty on version 3.x only on the finagle project or in a whole perspective (even 3rd party dependencies that use netty on an outdated version are having their netty dependency overriden).
If the issue that I've mentioned only takes effect on the finagle project, should it be considered a bump on the zookeeper version?

Thanks!

Alessandro Vermeulen
@spockz
@mosesn open telemetry is indeed new. But with adoption of spring and other jvm frameworks plus integration with opencensus and tracing it seems the most stable form. If at least the Tracer interface can be made compatible with open telemetry tracing it would be a benefit
Moses Nakamura
@mosesn
@spockz for sure! but my inclination would be to first build an OT adapter for c.t.f.tracing.Tracer instead of migrating Finagle to the OT API
@hllrsr netty 3 and netty 4 are published in different namespaces and under different package names, so it’s possible for a single program to depend on both netty 3 and netty 4. what’s the version you would want to bump ZK to?
Moses Nakamura
@mosesn

it looks like we should be able to bump to at least zk 3.5.5 safely, which I believe is on netty 4.x.

https://github.com/apache/zookeeper/blob/branch-3.5.5/build.xml#L40

hllrsr
@hllrsr
@mosesn I would sugest the nearest version from zk which is not using netty 3.x. In this case, you suggested zk 3.5.5 which should be fine
Is there a way I can help with this bump? i.e: open an issue or something like this
Moses Nakamura
@mosesn
yeah, can you make a PR against Finagle?
should just be bumping the version, I would be surprised if there was a breaking API change
hllrsr
@hllrsr
Sure, I will open the PR today.
Also, I'll try to run through changelogs on zookeeper to see if there is any breaking change
Moses Nakamura
@mosesn
thanks! and if you wouldn’t mind, if you could publish a local version of the updated Finagle and double check that it solves your problem, that would be great!
Alessandro Vermeulen
@spockz
@mosesn I tried building such an adapter I faced some challenges: controlling wgeee the spans were created. I think this is fixed now by the recreation of the span in the requeuefilter. Then there was the mismatch berwwwb the Tracer api letting the trace is be set externally which is not available in OT. And the third thing was cintrollling which attributes are placed on spans
hllrsr
@hllrsr
@mosesn In the process of compiling finagle with zk on version 3.5.5 I've got some type conversion errors. The latest version that didn't show any errors, only warnings was 3.5.4-beta.
How should I proceed in this case? Should I investigate why this errors are being caused or should I go with the 3.5.4-beta version?
Also, going through change logs they didn't mentioned any breaking change being introduced on 3.5.5.
hllrsr
@hllrsr
Those conversion errors were fixed on 3.5.6. I've manged to compile it on this version.
hllrsr
@hllrsr
I've published a local version and it solve my issue! I've also opened the PR on finagle. Thanks for the directions @mosesn :kudos:
Moses Nakamura
@mosesn
@hllrsr nice work! I commented on the PR
Moses Nakamura
@mosesn
hey folks, we did the January release today. enjoy 21.1.0!
TP Diffenbach
@tpdi
I have a question about Finagle Contexts and com.twitter.util.Local . Documentation suggests both are replacements for ThreadLocals across asynchronous Futures, but I'm not quite sure how to get there. Contexts cannot be updated, only let in order to change their values for execution of a function passed in to let. Locals can be updated, and the updated value is visible to child threads. But updates in child threads are not visible to siblings. That's strictly right in asynchronous programming I suppose, but I'm not sure how to emulate something like the synchronous process A updates ThreadLocal X to 1, process B reads 1 from ThreadLocal X.
In my case, I want parallel Futures in one request to share the same database transaction. If I stick the transaction in a LocalContext or a Context, and then create a bunch of async Futures, they all see the same transaction, which is great! But if transactions are started lazily by the first thread to need to, how can I ensure the other threads see that same transaction? And that different requests/contexts see only their own transaction? Thanks.
Christopher Coco
@cacoco

Does this help at all:

Finagle explicitly manages them for you across threads and execution contexts such as Future composition, FuturePools, Timers, and in some cases — across the client/server boundary.

(https://twitter.github.io/finagle/guide/Contexts.html)

let is a way of scoping a value in a closure
TP Diffenbach
@tpdi
I suppose I could add an extra level of indirection, allowing threads to write to mutable state in the context object, but I'm hoping for something cleaner
Christopher Coco
@cacoco
foo.let(something) { //everthing here sees the something value }
if you’re trying to coordinate state across a lot of futures...
TP Diffenbach
@tpdi
Right, I understand let. But I need updates within "//everthing here sees the something value" to be seen by all threads/Futures within that block
Christopher Coco
@cacoco
right we do this in places by mutating a value held in the context
(like how we propagate slf4j MDC values in Finatra)
TP Diffenbach
@tpdi
So I have in synchronous code some database access object that does a if (myThreadLocal.get() == null) myThreadLocal.set(new Transaction); and then subquestnt calls to it in that thread use the transaction
Ok, so you do use an extra level of indirection
the context is fixed, but the value it holds is mutable
Christopher Coco
@cacoco
exactly
TP Diffenbach
@tpdi
com.twitter.util.Local does almost what I want; in order to replace multiple ThreadLocals, I'll need to establish a key or index for each, com.twitter.util.Local does that, using a counter in the companion object that serves as the array index
I was hoping someone else had already had to do this ;)
Christopher Coco
@cacoco
hah, it is possible there’s something out there
TP Diffenbach
@tpdi
Out of curiosity, when do you use a Local rather than a LocalContext?
Christopher Coco
@cacoco
I think generally when we say local we mean a local context (at least I do)
TP Diffenbach
@tpdi
Sorry, I meant com.twitter.util.Local vs com.twitter.finagle.context.LocalContext.
But thanks so much for clarifying this for me
Alessandro Vermeulen
@spockz
So if you want to share a db transactionwithin the same calls to the database you set the context with .let(mydbkey, newAtomicRef)(... handler code that calls multiple dB calls). On access of the mydbkey you can make sure the transaction is created only once
Prateek Jain
@prateek13688

Hi .. I am new to Finagle and I am using it in my java service. However, I am facing some problem while implementing retries in case of certain HTTP Error codes. The retry policy shouldRetry function does not gets called in case of request failing with certain error codes
here is the snippet of the code i am using to instantiate the client

private Service<Request, Response> buildClient(String host) throws Exception {
    String hostString = String.format("%s:%s", host, "8080");
    PartialFunction<Tuple2<Request, Response>, ResponseClass> typed =
            new AbstractPartialFunction<Tuple2<Request, Response>, ResponseClass>() {
                @Override
                public boolean isDefinedAt(Tuple2<Request, Response> x) {
                    return x._2().status() == Status.InternalServerError() || x._2().status() == Status.ServiceUnavailable();
                    return true;
                }

                @Override
                public ResponseClass apply(Tuple2<Request, Response> x) {
                    return ResponseClass.RetryableFailure();
                }
            };

    return this.hostToClientMap.computeIfAbsent(host, s -> {
        try {
            return ClientBuilder.safeBuild(
                    ClientBuilder.get()
                            .stack(Http.client())
                            .hosts(hostString)
                            .hostConnectionLimit(serviceConfiguration.getHttp_max_conn_per_route())
                            .tcpConnectTimeout(Duration.fromMilliseconds(serviceConfiguration.getHttp_socket_timeout_in_ms()))
                            .keepAlive(true)
                            .tls(LogDispatcherFactory.buildSslContext(serviceConfiguration.getSsm_store_region(),
                                    serviceConfiguration.getPrism_certificate_path(),
                                    serviceConfiguration.getPrism_certificate_key_path(),
                                    serviceConfiguration.getTruststore_password()))
                            .connectTimeout(Duration.fromMilliseconds(serviceConfiguration.getHttp_connect_timeout_in_ms()))
                            .requestTimeout(Duration.fromMilliseconds(serviceConfiguration.getHttp_request_timeout_in_ms()))
                            .retryPolicy(new CustomRetryPolicy())
                            .retryBudget(RetryBudgets.newRetryBudget(Duration.fromSeconds(10), 10, 1))
                            .failFast(false)
                            .responseClassifier(HttpResponseClassifier.apply(typed))
                            .reportTo(DefaultStatsReceiver.get()));
        } catch (Exception e) {
            logger.error(String.format("Error initializing the finagle client for host %s.", hostString), e);
            return null;
        }
    });
}

Any thoughts on this ???

Alessandro Vermeulen
@spockz
Typically you should not use client builder anymore
If you use the methodbuilder from Http.method(“”) then there is a method to specify a partial function which tells the client to retry requests without marking that request as a failure
Moses Nakamura
@mosesn
I’d be interested in what kinds of error codes it doesn’t get called for. Finagle has a mechanism for automatically retrying (without asking the retry policy) if the remote peer has indicated that they are unable to serve the traffic because they’re overloaded
I agree with @spockz that you might want to use the more modern APIs instead of ClientBuilder. maybe check out MethodBuilder?
Prime
@PRIME_LINUX_twitter
where should i begin with ... i am trying to use twitter server for my own work
Alessandro Vermeulen
@spockz
@PRIME_LINUX_twitter maybe try Finatra or Finch. They are a bit more accessible
Jure Malovrh
@JureMalovrh

Hi!
I have some performance questions regarding Finagle. I understand that you should not do any CPU bound operation on Finagle threads and that you should offload I/O work to FuturePool. But checking the comment in FuturePool it states, that this Pool should mainly be used in I/O operations and that CPU-bound operations need some special treatment. Is there any documentation about this "special treatment"?
We currently have a Finagle server, that gets some data from the hash map, but if this data is not found in hashmap, we do a quite expensive regex parsing. Since regex is CPU bound and we are using FuturePool.unboundedPool, we ofter starve out the whole thread pool and our Finagle server becomes unresponsive. Is there any standard way to approach it? I was thinking that this regex parsing should only get a bounded future pool, for example 6 threads, so if there are a lot of cache misses, we should limit the blast radius, by only hogging 6 threads. Would this approach be viable? Or are there any other ways to check it? Should we downsize Finagle event loop threads to avoid hogging?

Thanks

5 replies
Yufan Gong
@yufangong
Hi all, just a heads up that we are starting the February release, we will be cross-building scala 2.11/2.12/2.13 this time, 2.11 will be dropped in March release. Thanks!