Skip to content

sql/opt/optbuilder: resolve remaining comments from #44015#44169

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
nvb:nvanbenschoten/sfuComments
Jan 22, 2020
Merged

sql/opt/optbuilder: resolve remaining comments from #44015#44169
craig[bot] merged 1 commit intocockroachdb:masterfrom
nvb:nvanbenschoten/sfuComments

Conversation

@nvb
Copy link
Copy Markdown
Contributor

@nvb nvb commented Jan 21, 2020

This commit resolves a few typos that were missed before #44015 was merged.

Release note: None

@nvb nvb requested a review from rytaft January 21, 2020 19:20
@nvb nvb requested a review from a team as a code owner January 21, 2020 19:20
@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

Copy link
Copy Markdown
Member

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

[nit] Consider also adding a comment to LockingItem mentioning that if the struct changes, the two methods in the interner need to be updated accordingly

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten and @rytaft)


pkg/sql/opt/optbuilder/locking.go, line 159 at r1 (raw file):

// ignoreLockingForCTE is a placeholder for the following comment:
//
// We intentionally do not propate any row-level locking information from the

propagate

@nvb nvb force-pushed the nvanbenschoten/sfuComments branch from e7c2b6b to 4f28b29 Compare January 21, 2020 20:23
Copy link
Copy Markdown
Contributor Author

@nvb nvb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider also adding a comment to LockingItem mentioning that if the struct changes, the two methods in the interner need to be updated accordingly

Done.

Thanks for the review Radu.

bors r+

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @rytaft)


pkg/sql/opt/optbuilder/locking.go, line 159 at r1 (raw file):

Previously, RaduBerinde wrote…

propagate

Done.

@otan
Copy link
Copy Markdown
Contributor

otan commented Jan 22, 2020

(i think bors failed this one because of license/cla not being triggered...)

@nvb
Copy link
Copy Markdown
Contributor Author

nvb commented Jan 22, 2020

Huh, let's try that again.

bors r+

@otan
Copy link
Copy Markdown
Contributor

otan commented Jan 22, 2020

I found you need to re-push so that the license/cla thing is triggered (i've found this before with my other PRs)

image

@nvb
Copy link
Copy Markdown
Contributor Author

nvb commented Jan 22, 2020

I see, thanks Oliver.

bors r-

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Jan 22, 2020

Canceled

This commit resolves a few typos that were missed before cockroachdb#44015
was merged.

Release note: None
@nvb nvb force-pushed the nvanbenschoten/sfuComments branch from 4f28b29 to 643371d Compare January 22, 2020 00:38
@nvb
Copy link
Copy Markdown
Contributor Author

nvb commented Jan 22, 2020

bors r+

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Jan 22, 2020

Build failed (retrying...)

craig bot pushed a commit that referenced this pull request Jan 22, 2020
43565: kvnemeses: begin scaffolding for a jepsen-style kv test r=nvanbenschoten/tbg a=danhhz

Package kvnemeses exercises the KV api with random traffic and then
validates that the observed behaviors are consistent with our
guarantees.

A set of Operations are generated which represent usage of the public KV
api. These include both "workload" operations like Gets and Puts as well
as "admin" operations like rebalances. These Operations can be handed to
an Applier, which runs them against the KV api and records the results.

Operations do allow for concurrency (this testing is much less
interesting otherwise), which means that the state of the KV map is not
recoverable from _only_ the input. TODO(dan): We can use RangeFeed to
recover the exact KV history. This plus some Kyle magic can be used to
check our transactional guarantees.

TODO (in later commits)
- Validate the log
- CPut/InitPut/Increment/Delete
- DeleteRange/ClearRange/RevertRange/Scan/ReverseScan
- ChangeReplicas/TransferLease
- ExportRequest
- AddSSTable
- Root and leaf transactions
- GCRequest
- Protected timestamps

Release note: None

44144: colexec: fix multiple starts of the wrapped processors r=yuzefovich a=yuzefovich

**colexec: fix multiple starts of the wrapped processors**

Previously, wrapped processors could be started multiple times if they
were in the input chain for the bufferOp (each of the CASE arms will
initialize its input - the bufferOp). Now this is fixed by tracking in
both Columnarizer and bufferOp whether Init has already been called.

Previous behavior could lead to a crash when rowexec.valuesProcessor was
wrapped because it sends a "bogus" metadata header on each call to
Start, and only single header is expected whereas with multiple Inits
they would be multiple headers.

Fixes: #44133.

Release note (bug fix): Previously, CockroachDB could crash in special
circumstances when vectorized execution engine is used (it was more
likely to happen if `vectorize=experimental_on` setting was used). Now
this has been fixed.

**execerror: catch panics coming from sql/execinfra package**

sql/execinfra is definitely a part of the vectorized engine as a whole,
so we should be catching panics coming from it when running vectorized
flows.

Release note: None

44169: sql/opt/optbuilder: resolve remaining comments from #44015 r=nvanbenschoten a=nvanbenschoten

This commit resolves a few typos that were missed before #44015 was merged.

Release note: None

44172: Include "/cockroach" into PATH in Docker image r=vladdy a=vladdy

This adds "/cockroach" into environment's PATH in Docker image to require less typing when invoking "cockroach" commands via running container.

Fixes: #44189

Co-authored-by: Daniel Harrison <daniel.harrison@gmail.com>
Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com>
Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
Co-authored-by: Vlad Artamonov <742047+vladdy@users.noreply.github.com>
@craig
Copy link
Copy Markdown
Contributor

craig bot commented Jan 22, 2020

Build succeeded

@craig craig bot merged commit 643371d into cockroachdb:master Jan 22, 2020
Copy link
Copy Markdown
Collaborator

@rytaft rytaft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Reviewed 1 of 2 files at r1, 2 of 2 files at r2.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale)

@nvb nvb deleted the nvanbenschoten/sfuComments branch January 23, 2020 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants