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

Use zeros instead of whitespaces as padding bytes #899

Merged
merged 2 commits into from
Feb 18, 2020

Conversation

drawlerr
Copy link
Contributor

It is more realistic to use 0-padding than whitespace padding in IDs, and this is the format Elasticsearch prefers anyways.

Applying @jpountz 's patch for ID generation, fixing associated tests.

Closes #896

@drawlerr drawlerr added the enhancement Improves the status quo label Feb 14, 2020
@drawlerr drawlerr self-assigned this Feb 14, 2020
@drawlerr drawlerr added bug Something's wrong and removed enhancement Improves the status quo labels Feb 14, 2020
Copy link
Contributor

@jpountz jpountz left a comment

Choose a reason for hiding this comment

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

Thanks @drawlerr !

@drawlerr
Copy link
Contributor Author

Arrg. Seems we've managed to coax out an unrelated race condition in Rally during the integration test. I've written this up in #900, but for now I'm just going to run tests again as we shouldn't be unlucky enough to hit it twice in a row:

@elasticsearchmachine run tests

@danielmitterdorfer
Copy link
Member

@elasticmachine test this please

@danielmitterdorfer danielmitterdorfer added the :Load Driver Changes that affect the core of the load driver such as scheduling, the measurement approach etc. label Feb 17, 2020
@danielmitterdorfer danielmitterdorfer added this to the 1.4.1 milestone Feb 17, 2020
Copy link
Member

@danielmitterdorfer danielmitterdorfer left a comment

Choose a reason for hiding this comment

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

Thanks for changing this. I'm fine with the actual change but:

  1. IMHO this is a breaking change: We should label it as such and add a small note to your migration docs.
  2. I don't think this is a bug but rather an enhancement?
  3. Once merged, can you please keep an eye on all our update-related workloads as their performance might be impacted by this change? If this is the case, can you please add an according annotation?

@drawlerr drawlerr added enhancement Improves the status quo and removed bug Something's wrong labels Feb 18, 2020
@drawlerr drawlerr merged commit d0bee8f into elastic:master Feb 18, 2020
@drawlerr drawlerr deleted the rally-896 branch February 18, 2020 19:02
@jpountz
Copy link
Contributor

jpountz commented Feb 18, 2020

Woohoo

@danielmitterdorfer danielmitterdorfer added the breaking Non-backwards compatible change label Feb 19, 2020
@tomcallahan tomcallahan changed the title Use zeroes instead of whitespaces as padding bytes Use zeros instead of whitespaces as padding bytes Feb 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Non-backwards compatible change enhancement Improves the status quo :Load Driver Changes that affect the core of the load driver such as scheduling, the measurement approach etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use zeroes instead of whitespaces as padding bytes
3 participants