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

[dnm] roachtest: add helper to set up jaeger and store data as artifacts #65844

Closed
wants to merge 1 commit into from

Conversation

tbg
Copy link
Member

@tbg tbg commented May 28, 2021

image

Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@tbg
Copy link
Member Author

tbg commented May 28, 2021

Wondering if this is useful. cc @andreimatei @nvanbenschoten @cockroachdb/test-eng

Copy link
Member

@nvanbenschoten nvanbenschoten left a comment

Choose a reason for hiding this comment

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

:lgtm: It looks very useful!

Reviewed 1 of 1 files at r1.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @tbg)


pkg/cmd/roachtest/kv.go, line 237 at r1 (raw file):

// The UI can be accessed on n1's port 16686.

Want to log this fact?


pkg/cmd/roachtest/kv.go, line 263 at r1 (raw file):

	m.Go(func(ctx context.Context) error {
		err := c.RunE(ctx, c.Node(1), `SPAN_STORAGE_TYPE=badger BADGER_EPHEMERAL=false \
BADGER_DIRECTORY_VALUE=logs/jaeger.badger/data BADGER_DIRECTORY_KEY=logs/jaeger.badger/key \

Is the logs directory the right thing to use? My concern is that this will be in the root filesystem, which is usually 10GB large on these systems. Meanwhile, the SSD mounted at /mnt/data1 is usually 375GB large. When you tested this out over the course of an hour, did you happen to take a look at how large the directory got?

Copy link
Member Author

@tbg tbg left a comment

Choose a reason for hiding this comment

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

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


pkg/cmd/roachtest/kv.go, line 263 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Is the logs directory the right thing to use? My concern is that this will be in the root filesystem, which is usually 10GB large on these systems. Meanwhile, the SSD mounted at /mnt/data1 is usually 375GB large. When you tested this out over the course of an hour, did you happen to take a look at how large the directory got?

I'll take a look. I think it makes sense to decide on a case-by-case basis. When we're debugging a short test that doesn't push many ops/sec, the log dir is practical because then the test harness fetches the data for us even when the cluster is destroyed, without having to add special fetching logic. If we wanted to check this in as always-on for some tests (maybe we want that, right?) I agree that we should steer clear of the boot partition.

Copy link
Member Author

@tbg tbg left a comment

Choose a reason for hiding this comment

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

Just ran the contention test again:

ubuntu@tobias-1622462130-01-n5cpu4-0001:/mnt/data1/jaeger.badger$ du -sch
37G	.
37G	total

Pretty hefty, but I guess we expected that.

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

@tbg
Copy link
Member Author

tbg commented Jul 6, 2021

Posting for later

	// Convenient copy-paste for local jaeger instance.
	const _ = `
docker run -d \
  --name jaeger \
  -v "$(pwd):/dump" \
  -e COLLECTOR_ZIPKIN_HOST_PORT=:9411 \
  -e SPAN_STORAGE_TYPE=badger \
  -e BADGER_EPHEMERAL=false \
  -e BADGER_DIRECTORY_VALUE=/dump/data \
  -e BADGER_DIRECTORY_KEY=/dump/key \
  -p 5775:5775/udp \
  -p 6831:6831/udp \
  -p 6832:6832/udp \
  -p 5778:5778 \
  -p 16686:16686 \
  -p 14268:14268 \
  -p 14250:14250 \
  -p 9411:9411 \
  jaegertracing/all-in-one:1.23`

@tbg tbg closed this Sep 9, 2022
tbg added a commit to tbg/cockroach that referenced this pull request Dec 20, 2022
This ends up being useful every now and then. I was just brushing it up
for cockroachdb#76147 and figured it
was about time to just land this.

This is a revival of
cockroachdb#65844, which I cannot
reopen at this point.

Epic: none
Release note: None
craig bot pushed a commit that referenced this pull request Dec 20, 2022
93933: sql: add `indnullsnotdistinct` column to `pg_catalog.pg_index` r=rafiss a=e-mbrown

Fixes: #92583

This commit `indnullsnotdistinct` column to pg_index. Since we do not support `NULLS [ NOT ] DISTINCT` on `CREATE INDEX` the column is also false.

Release note: None

93938: storage: fix `CheckSSTConflicts` intent conflict above MVCC range tombstone r=erikgrinaker a=erikgrinaker

`CheckSSTConflicts` could fail to handle a point key collision with an intent written above an MVCC range tombstone, because the intent was considered to be deleted by the MVCC range tombstone.

This patch makes `MVCCRangeKeyStack.Covered()` consider a timestamp of 0 (i.e. an intent) as being above all range keys, because intents must be above all range keys by definition. Otherwise, this condition in `CheckSSTConflicts` would consider the intent as a point key deleted by an MVCC range tombstone, and therefore ignored:

```go
extValueDeletedByRange := extHasRange && extHasPoint && extRangeKeys.Covers(extKey)
```

`AddSSTable` operations are only used on offline tables, so this won't affect transaction isolation for ongoing transactions. However, it is possible (but unlikely) for a committed but unresolved write intent left behind by a past transaction to not be considered for conflict handling, and for `AddSSTable` to write below it.

Resolves #93934.

Release note (bug fix): When experimental MVCC range tombstones are enabled (they're disabled by default), a bulk ingestion (e.g. an import) could fail to take a committed-but-unresolved write intent into account during conflict checks when written above an MVCC range tombstone. It was therefore possible in very rare circumstances for the ingestion to write a value below the timestamp of the committed intent, causing the ingested value to disappear.

93942: sql: add flag to allow resolving index on offline tables r=chengxiong-ruan a=chengxiong-ruan

Previously offline tables are always skipped when resolving an index. This pr adds an flag to allow offline tables to be considered. The main use case is for `SHOW RANGES` where index ranges of tables being imported (offline) should be shown.

Epic: None
Release note: None

93966: roachtest: add helper to set up jaeger and store data as artifacts r=tbg a=tbg

This ends up being useful every now and then. I was just brushing it up
for #76147 and figured it
was about time to just land this.

This is a revival of
#65844, which I cannot
reopen at this point.

Epic: none
Release note: None


Co-authored-by: e-mbrown <[email protected]>
Co-authored-by: Erik Grinaker <[email protected]>
Co-authored-by: Chengxiong Ruan <[email protected]>
Co-authored-by: Tobias Grieger <[email protected]>
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.

3 participants