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

checkpoint: fix empty map become nil after unmarshall #237

Merged
merged 3 commits into from Sep 6, 2019

Conversation

@lance6716
Copy link
Contributor

commented Sep 6, 2019

What problem does this PR solve?

fix empty map become nil after marshall and unmarshall

What is changed and how it works?

manually init these map. Seems it's a bug of marshall function provided by gogoproto.

Check List

Tests

  • Unit test, add an unit test. If we didn't init these map after unmarshall this test won't pass(hope I didn't make it wrong)

Side effects

Related changes

  • Need to cherry-pick to the release branch
@lance6716

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2019

/run-all-tests

PTAL @kennytm @GregoryIan

BTW patch branch for Tencent PCG is here and cherry-pick to 2.1 will be based on this pcg-debug

log.L().Error("checkpoint file is broken", zap.String("path", path), zap.Error(err2))
}
// FIXME: patch for empty map may need initialize manually, because currently
// FIXME: a map of zero size -> marshall -> unmarshall -> become nil, see checkpoint_test.go

This comment has been minimized.

Copy link
@GregoryIan

GregoryIan Sep 6, 2019

Collaborator

why using FIXME for these comments?

This comment has been minimized.

Copy link
@lance6716

lance6716 Sep 6, 2019

Author Contributor

I think it's an unexpected behaviour of gogo/protobuf's marshall and unmarshall. There's logic to recover a empty map(but didn't execute) in

if m.Engines == nil {
m.Engines = make(map[int32]*EngineCheckpointModel)

If gogo/protobuf fixed this bug, we won't bother to initiate. So I added a FIXME to remind to report bug and check if fixed from gogo/protobuf.

@GregoryIan

This comment has been minimized.

Copy link
Collaborator

commented Sep 6, 2019

Rest LGTM

Copy link
Member

left a comment

Rest LGTM

lightning/checkpoints/checkpoints_test.go Outdated Show resolved Hide resolved
@lance6716

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2019

/run-all-tests

@GregoryIan

This comment has been minimized.

Copy link
Collaborator

commented Sep 6, 2019

LGTM

@lance6716 lance6716 added status/LGT1 and removed status/PTAL labels Sep 6, 2019
@kennytm
kennytm approved these changes Sep 6, 2019
@kennytm kennytm added status/LGT2 and removed status/LGT1 labels Sep 6, 2019
@kennytm kennytm merged commit aaac2b4 into master Sep 6, 2019
4 checks passed
4 checks passed
coverage/coveralls Coverage decreased (-0.02%) to 77.813%
Details
idc-jenkins-ci-lightning/build Jenkins job succeeded.
Details
idc-jenkins-ci-tidb-lightning/test Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details
@kennytm kennytm deleted the fix-empty-map branch Sep 6, 2019
lance6716 added a commit that referenced this pull request Sep 6, 2019
* checkpoint: fix empty map become nil after unmarshall

* checkpoint: unify code style

* checkpoint: remove hard-coded path
lance6716 added a commit that referenced this pull request Sep 6, 2019
lance6716 added a commit that referenced this pull request Sep 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.