Skip to content
This repository has been archived by the owner on Nov 24, 2023. It is now read-only.

Add Lightning struct to replace loader #427

Closed
wants to merge 4 commits into from

Conversation

imtbkcat
Copy link

@imtbkcat imtbkcat commented Dec 13, 2019

What problem does this PR solve?

fix pingcap/tiflow#5833

What is changed and how it works?

Add Lightning struct implement Unit interface.
Implement Init and Process method.

Check List

Tests

  • No code

Code changes

  • Has exported variable/fields change

Side effects

  • Increased code complexity

Related changes

  • Need to be included in the release note

generate-dm.sh Outdated
@@ -3,7 +3,7 @@
cd dm/proto

echo "generate dm protobuf code..."
GOGO_ROOT=${GOPATH}/src/github.com/gogo/protobuf
GOGO_ROOT=/Users/imtbkcat/go/src/github.com/gogo/protobuf
Copy link
Contributor

Choose a reason for hiding this comment

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

😕

Copy link
Author

Choose a reason for hiding this comment

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

I use gvm to manage local go version, so GOPATH can not accessed by this shell script, I will fix this right now. 😂

@imtbkcat imtbkcat added priority/normal Minor change, requires approval from ≥1 primary reviewer status/DNM Do not merge, test is failing or blocked by another PR type/feature New feature labels Dec 13, 2019
Comment on lines +84 to +87
l.logCtx.L().Error("create MyDumperLoader failed", log.ShortError(err))
pr <- pb.ProcessResult{
Errors: []*pb.ProcessError{unit.NewProcessError(pb.ErrorType_UnknownError, err)},
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The code would be simpler if we could extract a function to log and send the error?

@WangXiangUSTC
Copy link
Contributor

/run-all-tests tidb=release-3.0

@codecov
Copy link

codecov bot commented Dec 17, 2019

Codecov Report

Merging #427 into master will increase coverage by 1.1888%.
The diff coverage is n/a.

@@               Coverage Diff                @@
##             master       pingcap/dm#427        +/-   ##
================================================
+ Coverage   56.2905%   57.4794%   +1.1888%     
================================================
  Files           166        165         -1     
  Lines         17248      16893       -355     
================================================
+ Hits           9709       9710         +1     
+ Misses         6585       6230       -355     
+ Partials        954        953         -1

@@ -0,0 +1,467 @@
<?xml version="1.0" encoding="UTF-8"?>
Copy link
Member

Choose a reason for hiding this comment

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

Do not commit this file to the repo?

@@ -330,6 +330,7 @@ enum UnitType {
Dump = 2;
Load = 3;
Sync = 4;
Lightning = 5;
Copy link
Member

Choose a reason for hiding this comment

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

Which one do you think is better:

  1. Add a new Lightning unit.
  2. Replace the implementation of Load unit with lightning.

lightningCf.App.TableConcurrency = cfg.PoolSize

// Set checkpoint config.
lightningCf.Checkpoint.Driver = lightningConfig.CheckpointDriverMySQL
Copy link
Member

Choose a reason for hiding this comment

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

How about adjusting more config items like Checkpoint.Schema (check all items one by one)?

@CLAassistant
Copy link

CLAassistant commented Mar 9, 2020

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ suzaku
❌ imtbkcat


imtbkcat seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@ti-chi-bot
Copy link
Member

@imtbkcat: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs-rebase priority/normal Minor change, requires approval from ≥1 primary reviewer size/XXL status/DNM Do not merge, test is failing or blocked by another PR status/Stale type/feature New feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace Loader with Lightning
7 participants