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

loader: generate db-schema-create.sql file if not exists #186

Merged
merged 6 commits into from
Jun 26, 2019

Conversation

WangXiangUSTC
Copy link
Contributor

What problem does this PR solve?

mydumper will not dump schema create sql sometimes, and then loader will return error.
fix issue https://internal.pingcap.net/jira/browse/TOOL-1316

What is changed and how it works?

create schema create sql file if not find

Check List

Tests

  • Unit test
  • Manual test

Related changes

  • Need to cherry-pick to the release branch

@WangXiangUSTC WangXiangUSTC added priority/normal Minor change, requires approval from ≥1 primary reviewer status/PTAL This PR is ready for review. Add this label back after committing new changes type/enhancement Performance improvement or refactoring labels Jun 25, 2019
@codecov
Copy link

codecov bot commented Jun 25, 2019

Codecov Report

Merging #186 into master will decrease coverage by 0.0428%.
The diff coverage is 43.75%.

@@               Coverage Diff                @@
##             master       #186        +/-   ##
================================================
- Coverage   53.2932%   53.2504%   -0.0429%     
================================================
  Files           121        121                
  Lines         13816      13829        +13     
================================================
+ Hits           7363       7364         +1     
- Misses         5709       5717         +8     
- Partials        744        748         +4

loader/util.go Outdated
}
defer file.Close()

file.WriteString(fmt.Sprintf("CREATE DATABASE `%s`;\n", schema))
Copy link
Contributor

Choose a reason for hiding this comment

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

Use fmt.Fprintf?

What if schema contains a `?

Copy link
Contributor

Choose a reason for hiding this comment

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

also need to check error returned from fmt.Fprintf or file.WriteString

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed in 25659ee

Copy link
Contributor Author

@WangXiangUSTC WangXiangUSTC Jun 26, 2019

Choose a reason for hiding this comment

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

do you have any suggestion when schema contains `? @kennytm

Copy link
Contributor

Choose a reason for hiding this comment

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

There should be a function somewhere which replaces ` with ``

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done in d87c959
PTAL again

loader/loader.go Outdated Show resolved Hide resolved
Copy link
Contributor

@kennytm kennytm left a comment

Choose a reason for hiding this comment

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

LGTM

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.

LGTM

@WangXiangUSTC WangXiangUSTC added status/LGT1 One reviewer already commented LGTM and removed status/PTAL This PR is ready for review. Add this label back after committing new changes labels Jun 26, 2019
@amyangfei amyangfei added status/LGT2 Two reviewers already commented LGTM, ready for merge and removed status/LGT1 One reviewer already commented LGTM labels Jun 26, 2019
@WangXiangUSTC WangXiangUSTC added status/LGT2 Two reviewers already commented LGTM, ready for merge and removed status/LGT2 Two reviewers already commented LGTM, ready for merge labels Jun 26, 2019
@WangXiangUSTC WangXiangUSTC merged commit 93123cd into master Jun 26, 2019
@WangXiangUSTC WangXiangUSTC deleted the xiang/refine_loader branch June 26, 2019 06:46
lichunzhu pushed a commit to lichunzhu/dm that referenced this pull request Apr 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
priority/normal Minor change, requires approval from ≥1 primary reviewer status/LGT2 Two reviewers already commented LGTM, ready for merge type/enhancement Performance improvement or refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants