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

ghost/changelog/del table names support max characters limit #451

Merged
merged 3 commits into from
Jul 20, 2017

Conversation

shlomi-noach
Copy link
Contributor

@shlomi-noach shlomi-noach commented Jul 19, 2017

With this PR it's OK for the original table name to be very long, and up to maxlen (64 characters).

The old, ghost and changelog names are truncated as needed to accommodate.

Furthermore, the truncation happens on the table name part, rather than on the suffix. To illustrate, this table name:

a12345678901234567890123456789012345678901234567890123456789012

is 63 characters long. The names gh-ost will pick are:

_a123456789012345678901234567890123456789012345678901234567_del
_a123456789012345678901234567890123456789012345678901234567_gho
_a123456789012345678901234567890123456789012345678901234567_ghc

Furthermore, if using --timestamp-old-table the suffix is even longer, and this truncates even more of the original name, e.g:

_a1234567890123456789012345678901234567890123_20130203195400_del

@shlomi-noach shlomi-noach merged commit d05f0ce into master Jul 20, 2017
@shlomi-noach shlomi-noach deleted the long-table-names branch July 20, 2017 13:47
context.StartTime, _ = time.Parse(longForm, "Feb 3, 2013 at 7:54pm (PST)")
oldTableName := context.GetOldTableName()
test.S(t).ExpectEquals(oldTableName, "_a1234567890123456789012345678901234567890123_20130203195400_del")
}
Copy link
Member

Choose a reason for hiding this comment

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

I am wondering in splitting this tests into different methods. When adding new tests here I got some mixed results because the context was mutated already.
WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@arthurnn sure!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants