Skip to content

feat(cdc) Set up structure to load snapshot into Clickhouse#398

Merged
fpacifici merged 7 commits into
masterfrom
feat/loadScaffold
Aug 9, 2019
Merged

feat(cdc) Set up structure to load snapshot into Clickhouse#398
fpacifici merged 7 commits into
masterfrom
feat/loadScaffold

Conversation

@fpacifici

Copy link
Copy Markdown
Contributor

This is the first step needed to be able to bulk load data into clickhouse.
The basic principle is that we will load data from files (one per table) into Snuba datasets, which, at present, are represented by one Clickhouse table. We are going to load data into a temp table and then do the swap.

This introduces:

  • the script that initiates the process
  • an abstraction over the snapshot source that is hopefully not too Postgres specific
  • ab abstraction that is able to load data from the source above into a table. Every dataset can provide its own loader.

@fpacifici
fpacifici requested a review from a team August 6, 2019 23:32
"""
return column_name

def get_bulk_loader(self, source, dest_table):

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Passing the source in input allows us to abstract the source away from the process to insert data into clickhouse. It also allows us to reuse the same instance if we decide to load multiple datasets at the same time.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Makes sense

Comment thread snuba/cli/bulk_load.py
help='The dataset to bulk load')
@click.option('--source',
help='Source of the dump. Depending on the dataset it may have different meaning.')
@click.option('--dest-table',

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'd like to make this more abstract but I believe we need to see how we will use this in prod before deciding wether we can hardcode the temporary table name in the dataset like the other table names. Same if we decide to put it into config. I would address that once we tried this in prod.

Comment thread snuba/snapshots/__init__.py Outdated
"""

@classmethod
def load(cls, path: str, product: str) -> BulkLoadSource:

@fpacifici fpacifici Aug 6, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

hey @tkaemming look! type hints in Snuba !
(will take care of mypy separately)

@tkaemming tkaemming left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Added some initial thoughts, generally this looks pretty good.

Comment thread setup.py
'pylint==1.9.2',
'pyparsing==2.2.0',
'pytest==3.6.1',
'pytest==3.9.3',

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Was this upgraded for a specific reason, or just trying to stay current?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

it is needed in order to use the "tmp_path" fixture in tests. It was introduced in 3.9

Comment thread snuba/cli/bulk_load.py
Comment thread snuba/cli/bulk_load.py
import sentry_sdk

sentry_sdk.init(dsn=settings.SENTRY_DSN)
logging.basicConfig(level=getattr(logging, log_level.upper()), format='%(asctime)s %(message)s')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should refactor this later so that the SDK doesn't need to be installed in each cli script independently.

Comment thread snuba/cli/bulk_load.py Outdated
Comment thread snuba/cli/bulk_load.py
Comment thread snuba/snapshots/postgres_snapshot.py Outdated
Comment thread snuba/snapshots/postgres_snapshot.py Outdated
Comment thread snuba/snapshots/postgres_snapshot.py Outdated
Comment thread snuba/snapshots/__init__.py Outdated
"""
return column_name

def get_bulk_loader(self, source, dest_table):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Makes sense

@tkaemming tkaemming left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks good. 👍



@dataclass(frozen=True)
class PostgresSnapshotDescriptor(SnapshotDescriptor):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is nice.

@fpacifici
fpacifici merged commit 74afa93 into master Aug 9, 2019
@fpacifici
fpacifici deleted the feat/loadScaffold branch September 13, 2019 05:39
phacops pushed a commit that referenced this pull request Jun 10, 2026
## Bump sentry-conventions to 0.11.0

Updates `sentry-conventions` from 0.10.0 to
[0.11.0](https://github.com/getsentry/sentry-conventions/releases/tag/0.11.0).

### Changes in 0.11.0

**New Features**
- Add `session.id` attribute (#412)
- Update several attributes to latest OTel representation (#418)
- Add `db.operation.batch.size` (#407)
- Add `app.vitals.start.prewarmed` attribute (#379)
- Add and deprecate runtime context attributes (#383)
- Add Android Runtime (ART) GC and memory attributes (#382)
- Add `db.stored_procedure.name` and ensure span name attribute
consistency (#398)
- Add Cloudflare SDK attributes (#392)
- Add missing GCP and FaaS attributes (#403)

**Bug Fixes**
- Set `pii: 'maybe'` on `faas` string attributes (#415)
- Add missing Cloudflare visibility attributes (#408)

**Internal**
- Remove `sdks` field from attribute schema and definitions (#410)
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.

2 participants