Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bazel: dev generate requires npm and yarn, even though it doesn't need to #76521

Closed
knz opened this issue Feb 14, 2022 · 0 comments · Fixed by #76550
Closed

bazel: dev generate requires npm and yarn, even though it doesn't need to #76521

knz opened this issue Feb 14, 2022 · 0 comments · Fixed by #76550
Assignees
Labels
A-build-system B-os-bsd Issues specific to a BSD OS (non-macOS) C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-dev-inf

Comments

@knz
Copy link
Contributor

knz commented Feb 14, 2022

Describe the problem

Discussed here: dev generate runs bazel query kind(go_proto_library, //pkg/...) which in turn chokes on #74208.

But there's no reason for bazel query to need that. As @rickystewart explains:

recursing into pkg/ui as part of the bazel query will cause Bazel to try to pull UI bits from NPM. In generate-test-suites we explicitly exclude ui and genbzl should probably be changed to do the same

Epic CRDB-8036

@knz knz added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. A-build-system labels Feb 14, 2022
@irfansharif irfansharif added the B-os-bsd Issues specific to a BSD OS (non-macOS) label Feb 14, 2022
@ajwerner ajwerner self-assigned this Feb 14, 2022
craig bot pushed a commit that referenced this issue Feb 15, 2022
…76591

75809: [CRDB-12226] server, ui: display circuit breakers in problem ranges and range status r=Santamaura a=Santamaura

This PR adds changes to the reports/problemranges and reports/range pages.
Ranges with replicas that have a circuit breaker will show up as problem ranges and
the circuit breaker error will show up as a row on the status page.

Release note (ui change): display circuit breakers in problems ranges and range status

Problem Ranges page:
![Screen Shot 2022-02-08 at 4 57 51 PM](https://user-images.githubusercontent.com/17861665/153082648-6c03d195-e395-456a-be00-55ad24863752.png)

Range status page:
![Screen Shot 2022-02-08 at 4 57 34 PM](https://user-images.githubusercontent.com/17861665/153082705-cbbe5507-e81d-49d7-a3f7-21b4c84226c2.png)



76278: Add cluster version as feature gate for block properties r=dt,erikgrinaker,jbowens a=nicktrav

Two commits here - the first adds a new cluster version, the second makes use of the cluster version as a feature gate (and update various call sites all over the place).

---

pkg/clusterversion: add new version as feature gate for block properties
Prior to this change, the block properties SSTable-level feature was
enabled in a single cluster version. This introduced a subtle race in
that for the period in which each node is being updated to
`PebbleFormatBlockPropertyCollector`, there is a brief period where not
all nodes are at the same cluster version, and thus store versions. If
nodes at the newer version write SSTables that make use of block
properties, and these tables are consumed by nodes that are yet to be
updated, the older nodes could panic when attempting to read the tables
with a format they do not yet understand.

While this race is academic, given that a) there are now subsequent
cluster versions that act as barriers during the migration, and b) block
properties were disabled in 1a8fb57, this patch addresses the race by
adding a second cluster version.
`EnablePebbleFormatVersionBlockProperties` acts as a barrier and a
feature gate. A guarantee of the migration framework is that any node at
this newer version is part of a cluster that has already run the
necessary migrations for the older version, and thus ratcheted the
format major version in the store, and thus enabled the block properties
feature, across all nodes.

Add additional documentation in `pebble.go` that details how to make use
of the two-version pattern for future table-level version changes.

---

pkg/storage: make MakeIngestionWriterOptions version aware
With the introduction of block properties, and soon, range keys, which
introduce backward-incompatible changes at the SSTable level, all nodes
in a cluster must all have a sufficient store version in order to avoid
runtime incompatibilities.

Update `storage.MakeIngestionWriterOptions` to add a `context.Context`
and `cluster.Settings` as parameters, which allows for determining
whether a given cluster version is active (via
`(clusterversion.Handle).IsActive()`). This allows gating the enabling /
disabling of block properties (and soon, range keys), on all nodes being
at a sufficient cluster version.

Update various call-sites to pass in the `context.Context` and
`cluster.Settings`.

---

76348: ui: downsample SQL transaction metrics using MAX r=maryliag a=dhartunian

Previously, we were using the default downsampling behavior of the
timeseries query engine for "Open SQL Transactions" and "Active SQL
Statements"  on the metrics page in DB console. This led to confusion
when zooming in on transaction spikes since the spike would get larger
as the zoom got tighter.

This PR changes the aggregation function to use MAX to prevent this
confusion.

Resolves: #71827

Release note (ui change): Open SQL Transactions and Active SQL
Transactions are downsampled using MAX instead of AVG and will more
accurately reflect narrow spikes in transaction counts when looking and
downsampled data.

76414: spanconfig: teach the KVAccessor about system span configurations r=arulajmani a=arulajmani

First 3 commits are from #76219, this one's quite small -- mostly just tests. 

----

This patch teaches the KVAccessor to update and get system span
configurations.

Release note: None

76538: ui: Use liveness info to populate decommissioned node lists r=zachlite a=zachlite

Previously, the decommissioned node lists considered node status entries
to determine decommissioning and decommissioned status. This changed in #56529,
resulting in empty lists. Now, the node's liveness entry is considered
and these lists are correctly populated.

Release note (bug fix): The list of recently decommissioned nodes
and the historical list of decommissioned nodes correctly display
decommissioned nodes.

76544: builtins: add rehome_row to DistSQLBlocklist r=mgartner,otan a=rafiss

fixes #76153

This builtin always needs to run on the gateway node.

Release note: None

76546: build: display pebble git SHA in GitHub messages r=rickystewart a=nicktrav

Use the short from of the Git SHA from the go.mod-style version in
DEPS.bzl as the Pebble commit. This ensures that failure messages
created by Team City link to a GitHub page that renders correctly.

Release note: None

76550: gen/genbzl: general improvements r=ajwerner a=ajwerner

This change does a few things:

 * It reworks the queries in terms of eachother in-memory. This is better than
   the previous iteration whereby it'd generate the results and then rely on
   the output of that query. Instead, we just build up bigger query expressions
   and pass them to bazel using the --query_file flag.
 * It avoids exploring the pkg/ui directory (and the pkg/gen directory) because
   those can cause problems. The pkg/ui directory ends up bringing in npm,
   which hurts.
 * It stops rewriting the files before executing the queries. It no longer
   needs to rewrite them up front because they aren't referenced by later
   queries.
 * It removes the excluded target which was problematic because those files
    weren't properly visible.

Fixes #76521
Fixes #76503

Release note: None

76562: ccl/sqlproxyccl: update PeekMsg to return message size instead of body size r=JeffSwenson a=jaylim-crl

Informs #76000. Follow-up to #76006.

Previously, PeekMsg was returning the body size (excluding header size), which
is a bit awkward from an API point of view because most callers of PeekMsg
immediately adds the header size to the returned size previously. This commit
cleans the API design up by making PeekMsg return the message size instead,
i.e. header inclusive. At the same time, returning the message size makes it
consistent with the ReadMsg API since that returns the entire message.

Release note: None

76591: bazel: update shebang line in `sql-gen.sh` r=rail a=rickystewart

Release note: None

Co-authored-by: Santamaura <[email protected]>
Co-authored-by: Nick Travers <[email protected]>
Co-authored-by: David Hartunian <[email protected]>
Co-authored-by: arulajmani <[email protected]>
Co-authored-by: Zach Lite <[email protected]>
Co-authored-by: Rafi Shamim <[email protected]>
Co-authored-by: Andrew Werner <[email protected]>
Co-authored-by: Jay <[email protected]>
Co-authored-by: Ricky Stewart <[email protected]>
@craig craig bot closed this as completed in 80b48cf Feb 15, 2022
@craig craig bot closed this as completed in #76550 Feb 15, 2022
RajivTS pushed a commit to RajivTS/cockroach that referenced this issue Mar 6, 2022
This change does a few things:

 * It reworks the queries in terms of eachother in-memory. This is better than
   the previous iteration whereby it'd generate the results and then rely on
   the output of that query. Instead, we just build up bigger query expressions
   and pass them to bazel using the --query_file flag.
 * It avoids exploring the pkg/ui directory (and the pkg/gen directory) because
   those can cause problems. The pkg/ui directory ends up bringing in npm,
   which hurts.
 * It stops rewriting the files before executing the queries. It no longer
   needs to rewrite them up front because they aren't referenced by later
   queries.
 * It removes the excluded target which was problematic because those files
   weren't properly visible.

Fixes cockroachdb#76521
Fixes cockroachdb#76503

Release note: None
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-build-system B-os-bsd Issues specific to a BSD OS (non-macOS) C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-dev-inf
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants