deps: upgrade Shopify/sarama v1.38.1 to IBM/sarama v1.42.1 #117544
Conversation
|
This change is |
|
Some commits that may need closer inspection: IBM/sarama#2672, IBM/sarama#2645, https://github.com/IBM/sarama/pull/2156/files, https://github.com/IBM/sarama/pull/2556/files, https://github.com/IBM/sarama/pull/2693/files, IBM/sarama#2484. The rest seems unrelevant. |
|
I tested this version with cdc/tpcc-1000 and seems to work expectedly. |
wenyihu6
requested review from
herkolategan,
DarrylWong and
miretskiy
and removed request for
a team,
herkolategan and
DarrylWong
|
Does anyone might know why the dependency for "github.com/Shopify/sarama" is still there although I've deleted every places in the codebase that imports it? 32c4548 Could it be because other packages still depend on it? |
miretskiy
requested review from
rharding6373 and
jayshrivastava
and removed request for
miretskiy
I think it's expected that you have to manually delete it from DEPS.bzl. If it keeps automatically coming back, then I would ask dev inf about it. Also this LGTM, but technically it needs a TL to approve it. They are still using the MIT license, so it should be okay to merge. I think the testing is sufficient enough to say that this won't cause problems. |
How does this compare to using the previous version of sarama? Could you point me to where the latest benchmark numbers are? |
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @jayshrivastava and @wenyihu6)
DEPS.bzl line 7882 at r2 (raw file):
importpath = "github.com/Shopify/sarama", sha256 = "ca251ac94fc78893afd2c2debf9ae061223ff4cb174daa508e2b0146f66de40e", strip_prefix = "github.com/Shopify/[email protected]",
Interesting that this is not the same version that you deleted..
There was a problem hiding this comment.
Reviewable status:
DEPS.bzl line 7882 at r2 (raw file):
Previously, rharding6373 (Rachael Harding) wrote…
Interesting that this is not the same version that you deleted..
It looks like there's a dependency from "github.com/bsm/sarama-cluster" to "github.com/Shopify/sarama", which could potentially be causing this. Interestingly, sarama-cluster is deprecated, and I can't find any other uses of it in the code base. Do you happen to know what we use it for?
![blathers[crl]](https://avatars.githubusercontent.com/in/59700?s=40&u=373d9101a12edc3847d2fb6ae78e3adcfb4ca41e&v=4){kind=avatar}
|
Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
Previously, rharding6373 (Rachael Harding) wrote…
Good point. I tried looking into this more. It looks like there are some other packages that depend on the previous version of sarama which caused the old sarama package to stay. I'm not sure if we want to upgrade those packages as well. Their latest version also depend on the previous version of sarama. |
|
@rharding6373 Good point. I vaguely remember this discussion when we were discussing this PR. iirc, I don't think we have a benchmark numbers that could be reliably used for performance comparison. We can run more tests and closely introspect different metrics more. |
There was a problem hiding this comment.
RE: "seems to work expectedly" with benchmark results -- My real question (which is mostly educational) is what does "work expectedly" mean in this context? The benchmark successfully built and ran? The latency and tput #s are comparable to before the change? They're below some threshold that people familiar with the benchmark would find egregious? I don't think this is a change that necessarily needs detailed benchmarking. I'm just curious. Sorry for digging into an old comment!
Reviewable status:
-- commits line 4 at r3:
Maybe update the description to indicate the an older version of sarama is kept to satisfy other dependencies by third party libraries.
DEPS.bzl line 7882 at r2 (raw file):
Previously, wenyihu6 (Wenyi Hu) wrote…
Good point. I tried looking into this more. It looks like there are some other packages that depend on the previous version of sarama which caused the old sarama package to stay.
❯ go mod graph | grep github.com/Shopify/sarama github.com/jaegertracing/[email protected] github.com/Shopify/[email protected] github.com/openzipkin/[email protected] github.com/Shopify/[email protected] github.com/openzipkin/[email protected] github.com/Shopify/[email protected] github.com/openzipkin/[email protected] github.com/Shopify/[email protected] github.com/openzipkin/[email protected] github.com/Shopify/[email protected]I'm not sure if we want to upgrade those packages as well. Their latest version also depend on the previous version of sarama.
Nice sleuthing.
|
TestChangefeedEndTimeWithCursor/webhook just failed in CI which looks like a flake to me. I will try repro again here and on master. |
I meant "worked expectedly" as the test |
|
I was able to repro this test flake TestChangefeedEndTimeWithCursor/webhook on master pretty consistently with |
|
Thanks for the explanation! |
This patch updates sarama library to the latest version. Note that the ownership of the sarama library has been transferred from Shopify to IBM. Fixes: cockroachdb#117522 Release note: none
|
TFTRs! bors r=rharding6373 The last two pushes was empty - just to trigger CI to avoid the known test flakes. |
|
Build succeeded: |
Now that this patch (cockroachdb#117544) has been merged, sarama now acknowledges and reacts to kafka server's throttling messages by slowing down. To provide better observability into sarama code, this patch adds a metrics registry interceptor and a new metrics `changefeed.kafka_throttling_hist_nanos` which tracks time (in nanos) spent in sarama's throttling when cockroachdb exceed the kafka quota. Fixes: cockroachdb#117618 Release note: changefeed.kafka_throttling_hist_nanos has now been added to metrics to monitor sarama throttling behavior resulting from exceeding kafka quota.
Now that this patch (cockroachdb#117544) has been merged, sarama now acknowledges and reacts to kafka server's throttling messages by slowing down. To provide better observability into sarama code, this patch adds a metrics registry interceptor and a new metrics `changefeed.kafka_throttling_hist_nanos` which tracks time (in nanos) spent in sarama's throttling when cockroachdb exceed the kafka quota. Fixes: cockroachdb#117618 Release note: changefeed.kafka_throttling_hist_nanos has now been added to metrics to monitor sarama throttling behavior resulting from exceeding kafka quota.
Now that this patch (cockroachdb#117544) has been merged, sarama now acknowledges and reacts to kafka server's throttling messages by slowing down. To provide better observability into sarama code, this patch adds a metrics registry interceptor and a new metrics `changefeed.kafka_throttling_hist_nanos` which tracks time (in nanos) spent in sarama's throttling when cockroachdb exceed the kafka quota. Fixes: cockroachdb#117618 Release note: changefeed.kafka_throttling_hist_nanos has now been added to metrics to monitor sarama throttling behavior resulting from exceeding kafka quota.
117117: kv: log slow requests on replica level in addition to range level r=shralex a=shralex Previously, slow requests were only logged at the range level, but the logs did not indicate which replica is slow. Moreover, the SlowRPC metric attempted to represent the number of requests currently being retried, however it was done on the range level and therefore missed a second level of replica-level retries being done underneath. This PR adds logging on the replica level, removes a confusing log line, and changes the metric to count the number of slow requests in a simpler manner. Epic: https://cockroachlabs.atlassian.net/browse/CRDB-33510 Fixes: #114431 117693: changefeedccl: add observability metrics into sarama code r=rharding6373 a=wenyihu6 Now that this patch (#117544) has been merged, sarama now acknowledges and reacts to kafka server's throttling messages by slowing down. To provide better observability into sarama code, this patch adds a metrics registry interceptor and a new metrics `changefeed.kafka_throttling_hist_nanos` which tracks time (in nanos) spent in sarama's throttling when cockroachdb exceed the kafka quota. Fixes: #117618 Release note: changefeed.kafka_throttling_hist_nanos has now been added to metrics to monitor sarama throttling behavior resulting from exceeding kafka quota. 118372: sql: fix flake in TestTxnContentionEventsTable r=yuzefovich a=michae2 In causeContention we deliberately hold a transaction open using pg_sleep to block an update statement. The timing we're trying to achieve is: 1. transaction insert 2. update starts and blocks 3. transaction held open using pg_sleep We were using a WaitGroup to order (2) after (1), but there was no synchronization to ensure (3) came after (2). This commit adds a retry loop that checks `crdb_internal.cluster_queries` to ensure (3) comes after (2). Fixes: #118236 Release note: None 118760: builtins: allow VIEWACTIVITY priv to use crdb_internal.request_statem… r=xinhaoz a=xinhaoz …ent_bundle Previously only those with the VIEWACTIVITY role could use the crdb_internal.request_statement_bundle builtin. We should allow the VIEWACTIVITY privilege as well since role options are now deprecated. This allow also allow stmt bundle requests to be made from db-console for users with this granted privilege. Epic: none Fixes: #118759 Release note (bug fix): Those with VIEWACTIVITY privilege can now request statement bundles using crdb_internal.requets_statement_bundle or via db-console's sql activity page. 118767: release: confirm yum install r=celiala a=rail This adds `-y` flag to install `yum` without user prompt. Epic: none Release note: None 118789: jobs,application_api: replace calls to `skip.Stress` with `skip.Duress` r=celiala a=rickystewart `skip.Duress()` seems like it should have been used in this case as it gives more time under both `race` and `deadlock`. This will give these tests some extra time if they run in a heavyweight configuration but not "under stress". Epic: CRDB-8308 Release note: None 118792: kvfollowerreadsccl: skip test under `race` not `stressrace` r=celiala a=rickystewart Epic: CRDB-8308 Release note: None 118797: bincheck: do not run geos tests on Windows r=celiala a=rail In #106642 we stopped shipping libgeos on Windows, but didn't update the bincheck test to reflect the change. Epic: none Release note: None Co-authored-by: shralex <[email protected]> Co-authored-by: Wenyi Hu <[email protected]> Co-authored-by: Michael Erickson <[email protected]> Co-authored-by: Xin Hao Zhang <[email protected]> Co-authored-by: Rail Aliiev <[email protected]> Co-authored-by: Ricky Stewart <[email protected]>
Now that this patch (cockroachdb#117544) has been merged, sarama now acknowledges and reacts to kafka server's throttling messages by slowing down. To provide better observability into sarama code, this patch adds a metrics registry interceptor and a new metrics `changefeed.kafka_throttling_hist_nanos` which tracks time (in nanos) spent in sarama's throttling when cockroachdb exceed the kafka quota. Fixes: cockroachdb#117618 Release note: changefeed.kafka_throttling_hist_nanos has now been added to metrics to monitor sarama throttling behavior resulting from exceeding kafka quota.
This patch updates sarama library to the latest version. Note that the ownership
of the sarama library has been transferred from Shopify to IBM.
Fixes: #117522
Release note: none
Here is the list of commits between the two versions upgrade.