Skip to content

Conversation

senior
Copy link
Contributor

@senior senior commented Dec 18, 2015

The terminus includes code to help users diagnose the source of data
that cannot be converted to UTF-8. There are several sources of this,
but one is having an incorrect (or known) character set for portions of
the catalog. When this invalid character data is large, it would cause
the terminus to hang trying to include debugging information.

This patch changes the terminus to only look for the first instance of
bad data. It will also avoid doing the extra calculations needed for the
error context unless debug mode is enabled. When not in debug mode there
should be no impact on performance.

The terminus includes code to help users diagnose the source of data
that cannot be converted to UTF-8. There are several sources of this,
but one is having an incorrect (or known) character set for portions of
the catalog. When this invalid character data is large, it would cause
the terminus to hang trying to include debugging information.

This patch changes the terminus to only look for the first instance of
bad data. It will also avoid doing the extra calculations needed for the
error context unless debug mode is enabled. When not in debug mode there
should be no impact on performance.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This new code only checks the index of the first invalid character, just like https://github.com/puppetlabs/puppetdb/pull/1789/files#diff-a0488ae759c41269703997e51a97b388R102

@senior
Copy link
Contributor Author

senior commented Dec 18, 2015

@vzctl @olivierHa can you try this out and see if it fixes your problem? I used a manifest like the one described here and it's working fine for me.

@puppetlabs-jenkins
Copy link
Contributor

Refer to this link for build results (access rights to CI server needed):
http://jenkins-enterprise.delivery.puppetlabs.net/job/enterprise_puppetdb_init-multijob_githubpr-stable/103/
Test PASSed.

@vzctl
Copy link

vzctl commented Dec 18, 2015

Yes, it fixes the problem, there are no performance issues with and without debug output. Thanks @senior for fixing it up!

@ajroetker
Copy link
Contributor

Testing this now 💃

@ajroetker
Copy link
Contributor

Works for me using the catalogs for testing the original patch as well as some new ones generated per the ticket description 👍, any idea what's going on with travis @senior?

ajroetker added a commit that referenced this pull request Dec 18, 2015
…ta-causing-terminus-hangs

(PDB-2256) Fix terminus bug with large binary catalog data
@ajroetker ajroetker merged commit 513f493 into puppetlabs:stable Dec 18, 2015
@ajroetker
Copy link
Contributor

Travis looks to be an unrelated timeout, gonna merge this in

@kbarber kbarber added this to the 3.2.3 (stable) milestone Dec 20, 2015
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.

5 participants