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

loader/syncer: fix bug in #355 #381

Merged
merged 5 commits into from
Nov 29, 2019
Merged

Conversation

lichunzhu
Copy link
Contributor

What problem does this PR solve?

Son PR of #355.
log.ShortError(err) may contains extra msg for context.Canceled.

What is changed and how it works?

change == to strings.Contains

Check List

Tests

  • Unit test
  • Integration test

@lichunzhu lichunzhu 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 labels Nov 28, 2019
@codecov
Copy link

codecov bot commented Nov 28, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@ffd159f). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff             @@
##             master      #381   +/-   ##
==========================================
  Coverage          ?   57.534%           
==========================================
  Files             ?       163           
  Lines             ?     16432           
  Branches          ?         0           
==========================================
  Hits              ?      9454           
  Misses            ?      6054           
  Partials          ?       924

pkg/log/log.go Outdated Show resolved Hide resolved
@lichunzhu
Copy link
Contributor Author

/run-all-tests tidb=release-3.0

@lichunzhu
Copy link
Contributor Author

@csuzhangxc PTAL

pkg/log/log.go Outdated
return
}
if field.Type == zapcore.ErrorType {
Copy link
Member

Choose a reason for hiding this comment

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

  1. How about using a switch to check filed.Type first?
  2. Can you add a unit test case for this? ref https://github.com/pingcap/tidb-lightning/blob/605760d1b2025d1e1a8b7d0c668c74863d7d1271/lightning/log/log_test.go#L43.

"go.uber.org/zap/zaptest"
)

// MakeTestLogger creates a Logger instance which produces JSON logs.
Copy link
Contributor

Choose a reason for hiding this comment

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

how add a note this function only used for test?

Copy link
Contributor

Choose a reason for hiding this comment

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

or we can move it to test file

@lichunzhu
Copy link
Contributor Author

/run-all-tests tidb=release-3.0

@lichunzhu
Copy link
Contributor Author

@WangXiangUSTC Revised. PTAL

@WangXiangUSTC
Copy link
Contributor

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 Nov 29, 2019
Copy link
Member

@csuzhangxc csuzhangxc left a comment

Choose a reason for hiding this comment

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

LGTM

@csuzhangxc csuzhangxc added status/LGT2 Two reviewers already commented LGTM, ready for merge and removed status/LGT1 One reviewer already commented LGTM labels Nov 29, 2019
@csuzhangxc csuzhangxc added this to the v1.0.3 milestone Nov 29, 2019
@lichunzhu lichunzhu merged commit 7f1e8e2 into pingcap:master Nov 29, 2019
@lichunzhu lichunzhu deleted the czli/hotfixPR355 branch November 29, 2019 08:31
@ericsyh ericsyh removed this from the v1.0.3 milestone Dec 3, 2019
lichunzhu added a commit to lichunzhu/dm that referenced this pull request Apr 6, 2020
* fix situations when there is extra msg in error.Error

* add deal for zap.Error and add UT
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants