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

Support write TiCDC log to S3. #826

Merged
merged 48 commits into from
Aug 13, 2020
Merged

Support write TiCDC log to S3. #826

merged 48 commits into from
Aug 13, 2020

Conversation

3pointer
Copy link
Contributor

@3pointer 3pointer commented Aug 5, 2020

What problem does this PR solve?

part of #768, support incremental log and write log data to s3 directly.

What is changed and how it works?

  1. Define cdclog with three kind of events.

    • row changed event: translate to protobuf then write data into cdclog
    • ddl event: translate to protobuf then write data into ddlog
    • resolved ts event: remain global resolved ts to log.meta
  2. Since s3 doesn't allow append-like operation. there are three different treatments for different events

    • row changed event: flush data to cdclog, if flushed data size is less than 5M. directly write to s3 with putObject request. if flushed data size is greater than 5M, use multi-upload request to upload data as one chunk in s3. if total upload chunks size is greater than 100M, then complete and rotate it. use the last row event's commit ts as the suffix of each cdclog name.
    • ddl event: consider ddl is not frequent in most cases. when ddllog size less than 10M, I choose to read content, append new ddl event then write back to ddllog.
    • resolved ts event: update in every EmitCheckpoinTs inplace.

Check List

Tests

  • Manual test (add detailed scripts or steps below)

屏幕快照 2020-08-06 下午2 04 55

屏幕快照 2020-08-06 下午2 05 04

Side effects

  • Possible performance regression
  • Increased code complexity
  • Breaking backward compatibility

Related changes

  • Need to cherry-pick to the release branch
  • Need to update the documentation
  • Need to update the tidb-cdc/cdc-ansible

Release note

@CLAassistant
Copy link

CLAassistant commented Aug 5, 2020

CLA assistant check
All committers have signed the CLA.

@3pointer 3pointer requested a review from zier-one August 5, 2020 13:24
@3pointer 3pointer changed the title Log sink Support write TiCDC log to S3. Aug 6, 2020
cdc/model/sink.go Outdated Show resolved Hide resolved
proto/LogProtocol.proto Outdated Show resolved Hide resolved
proto/LogProtocol.proto Outdated Show resolved Hide resolved
cdc/processor.go Outdated
@@ -1024,6 +1024,11 @@ func runProcessor(
cancel()
return nil, errors.Trace(err)
}
err = sink.Initialize(ctx, nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems initialize does nothing in processor, is it better to remove it?

}
data, err := row.ToProtoBuf().Marshal()
if err != nil {
errCh <- err
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to return when an error happens?

if err != nil {
errCh <- err
}
rowDatas = append(rowDatas, append(data, '\n')...)
Copy link
Contributor

Choose a reason for hiding this comment

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

is \n used as row delimiter, what happens when data contains \n

cdc/sink/cdclog/file.go Outdated Show resolved Hide resolved
cdc/sink/cdclog/s3.go Outdated Show resolved Hide resolved
cdc/sink/cdclog/s3.go Outdated Show resolved Hide resolved
cdc/sink/cdclog/file.go Outdated Show resolved Hide resolved
cdc/sink/cdclog/s3.go Show resolved Hide resolved
@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 12, 2020
@@ -442,6 +442,7 @@ func (m *mounterImpl) mountRowKVEntry(tableInfo *model.TableInfo, row *rowKVEntr
Table: &model.TableName{
Schema: schemaName,
Table: tableName,
TableID: row.PhysicalTableID,
Partition: partitionID,
Copy link
Contributor

Choose a reason for hiding this comment

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

we can remove the Partition now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about remove the Partition in another PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

ok

@3pointer
Copy link
Contributor Author

/run-integration-tests

@zier-one
Copy link
Contributor

/lgtm

@ti-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Aug 13, 2020
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Aug 13, 2020
if err != nil {
return err
}
file, err := os.OpenFile(filepath.Join(tableDir, fileName), os.O_CREATE|os.O_WRONLY|os.O_APPEND, defaultFileMode)
Copy link
Contributor

Choose a reason for hiding this comment

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

The new file has the same file name with the rotated file?

Copy link
Contributor Author

@3pointer 3pointer Aug 13, 2020

Choose a reason for hiding this comment

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

No, when batch events flushed, it will create new file name here, but not every new file name will be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

eh... maybe we should use the first event' commit ts as the filename

Copy link
Contributor

@amyangfei amyangfei left a comment

Choose a reason for hiding this comment

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

Also need test for file sink, we can do it in another PR

@3pointer
Copy link
Contributor Author

/run-integration-tests

@3pointer
Copy link
Contributor Author

/run-integration-tests

@3pointer
Copy link
Contributor Author

/run-integration-tests

@ti-srebot ti-srebot added status/LGT3 The PR has already had 3 LGTM. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Aug 13, 2020
@amyangfei
Copy link
Contributor

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Aug 13, 2020
@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@3pointer merge failed.

@3pointer
Copy link
Contributor Author

/run-all-tests

@3pointer 3pointer merged commit 6753924 into pingcap:master Aug 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/sink Sink component. status/can-merge Indicates a PR has been approved by a committer. status/LGT3 The PR has already had 3 LGTM.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants