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

Issue 490: Add a flag to not serialize ctime field #718

Closed
wants to merge 1 commit into from

Conversation

sijie
Copy link
Member

@sijie sijie commented Nov 11, 2017

Descriptions of the changes in this PR:

This request is for DL bookkeeper version upgrade. Because twitter's bk version doesn't include this change, so any ledgers written by new bk version will fail to be read by old version. To simplify the upgrade story, it is good to allow not serialize ctime field.

This request is for DL bookkeeper version upgrade. Because twitter's bk version doesn't include this change, so any ledgers written by new bk version will fail to be read by old version. To simplify the upgrade story, it is good to allow not serialize ctime field.
@sijie
Copy link
Member Author

sijie commented Nov 11, 2017

This is the pull request for #490

@sijie
Copy link
Member Author

sijie commented Nov 11, 2017

@eolivelli since the ctime feature was added by you, I am adding a flag for it. I turn it off to simplify the upgrade story. it should not impact your logic, because it will fall back to use the ctime in metadata store. please check and let me know if it is okay.

@asfgit
Copy link

asfgit commented Nov 11, 2017

FAILURE

--none--

Copy link
Contributor

@merlimat merlimat left a comment

Choose a reason for hiding this comment

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

👍 I would also say that we should consider switching to binary protobuf format for metadata, which doesn't suffer from schema evolution problems.

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

+1 looks good

@sijie
Copy link
Member Author

sijie commented Nov 11, 2017

@merlimat +1 on switching to binary format.

@eolivelli
Copy link
Contributor

+1 for me to for binary format

Copy link
Member

@jiazhai jiazhai left a comment

Choose a reason for hiding this comment

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

+1

@jiazhai jiazhai closed this in a9b64e6 Nov 12, 2017
jiazhai pushed a commit that referenced this pull request Nov 12, 2017
Descriptions of the changes in this PR:

This request is for DL bookkeeper version upgrade. Because twitter's bk version doesn't include this change, so any ledgers written by new bk version will fail to be read by old version. To simplify the upgrade story, it is good to allow not serialize ctime field.

Author: Sijie Guo <sijie@apache.org>

Reviewers: Enrico Olivelli <eolivelli@gmail.com>, Jia Zhai <None>, Sijie Guo <sijie@apache.org>, Matteo Merli <mmerli@apache.org>

This closes #718 from sijie/flag_to_not_serialize_ctime, closes #490

(cherry picked from commit a9b64e6)
Signed-off-by: Jia Zhai <zhaijia@apache.org>
@sijie sijie modified the milestones: 4.7.0, 4.6.0 Nov 22, 2017
philipsu522 pushed a commit to philipsu522/bookkeeper that referenced this pull request Dec 8, 2017
Descriptions of the changes in this PR:

This request is for DL bookkeeper version upgrade. Because twitter's bk version doesn't include this change, so any ledgers written by new bk version will fail to be read by old version. To simplify the upgrade story, it is good to allow not serialize ctime field.

Author: Sijie Guo <sijie@apache.org>

Reviewers: Enrico Olivelli <eolivelli@gmail.com>, Jia Zhai <None>, Sijie Guo <sijie@apache.org>, Matteo Merli <mmerli@apache.org>

This closes apache#718 from sijie/flag_to_not_serialize_ctime, closes apache#490
aojea pushed a commit to aojea/bookkeeper that referenced this pull request Dec 16, 2017
Descriptions of the changes in this PR:

This request is for DL bookkeeper version upgrade. Because twitter's bk version doesn't include this change, so any ledgers written by new bk version will fail to be read by old version. To simplify the upgrade story, it is good to allow not serialize ctime field.

Author: Sijie Guo <sijie@apache.org>

Reviewers: Enrico Olivelli <eolivelli@gmail.com>, Jia Zhai <None>, Sijie Guo <sijie@apache.org>, Matteo Merli <mmerli@apache.org>

This closes apache#718 from sijie/flag_to_not_serialize_ctime, closes apache#490
@sijie sijie deleted the flag_to_not_serialize_ctime branch July 16, 2018 02:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants