Class Parameters, Performance tuning #142

Closed
wants to merge 6 commits into
from

Conversation

Projects
None yet
5 participants
@fhrbek
Contributor

fhrbek commented Dec 4, 2012

This is a new pull request for class parameters and performance tuning, this time based on the correct origin.

@sodabrew

This comment has been minimized.

Show comment Hide comment
@sodabrew

sodabrew Dec 4, 2012

Owner

Thanks, the diffs look a lot better! Time to start reading code :)

Owner

sodabrew commented Dec 4, 2012

Thanks, the diffs look a lot better! Time to start reading code :)

@sodabrew

This comment has been minimized.

Show comment Hide comment
@sodabrew

sodabrew Dec 7, 2012

Owner

Does this enable the ENC yaml output for parameterized classes? Yes, that's exactly what it does. Very cool! According to http://docs.puppetlabs.com/guides/external_nodes.html#enc-output-format all puppet clients of this dashboard would have to be running Puppet 2.6.5 or higher.

Owner

sodabrew commented Dec 7, 2012

Does this enable the ENC yaml output for parameterized classes? Yes, that's exactly what it does. Very cool! According to http://docs.puppetlabs.com/guides/external_nodes.html#enc-output-format all puppet clients of this dashboard would have to be running Puppet 2.6.5 or higher.

@haus

This comment has been minimized.

Show comment Hide comment
@haus

haus Dec 20, 2012

Contributor

@sodabrew just the master needs to be of that version to parse the enc. clients of the master need to be no more than one major release behind the master, however.

Contributor

haus commented Dec 20, 2012

@sodabrew just the master needs to be of that version to parse the enc. clients of the master need to be no more than one major release behind the master, however.

@sodabrew

This comment has been minimized.

Show comment Hide comment
@sodabrew

sodabrew Dec 20, 2012

Owner

Ok, cool. I'm not on top of this changeset yet, but looking at it next.

@fhrbek Could you squash some of the commits together? In particular I think 17900 and 17901 should meld into 17899.

Owner

sodabrew commented Dec 20, 2012

Ok, cool. I'm not on top of this changeset yet, but looking at it next.

@fhrbek Could you squash some of the commits together? In particular I think 17900 and 17901 should meld into 17899.

@fhrbek

This comment has been minimized.

Show comment Hide comment
@fhrbek

fhrbek Dec 20, 2012

Contributor

@sodabrew I can do that but is it necessary? Current commits match redmine issues. Let me know if you want to squash them anyway.

Contributor

fhrbek commented Dec 20, 2012

@sodabrew I can do that but is it necessary? Current commits match redmine issues. Let me know if you want to squash them anyway.

@sodabrew

This comment has been minimized.

Show comment Hide comment
@sodabrew

sodabrew Dec 20, 2012

Owner

Oh, I didn't catch that they were redmine issues. Lemme take a look... Well, it looks like there are three major things happening:

  • Class parameters
  • Better conflict resolution (does this also improve group-group parameter conflicts?)
  • Update jQuery plugin
Owner

sodabrew commented Dec 20, 2012

Oh, I didn't catch that they were redmine issues. Lemme take a look... Well, it looks like there are three major things happening:

  • Class parameters
  • Better conflict resolution (does this also improve group-group parameter conflicts?)
  • Update jQuery plugin
@fhrbek

This comment has been minimized.

Show comment Hide comment
@fhrbek

fhrbek Dec 20, 2012

Contributor

Yes, group conflicts are also handled.
There's yet another fixed issue regarding performance with large data - I found out that using OpenStruct is a performance killer so I changed them to ruby hashes (much much faster).
What we did not finish was the design - now we have two different kinds of pop-ups - alerts and rendered windows (which need more attention regarding styling). We need to hear some opinion and then finish this part (e.g. unify all pop-ups as rendered windows and get them designed by a real designer which is something I'd recommend rather than using ugly alerts).

Contributor

fhrbek commented Dec 20, 2012

Yes, group conflicts are also handled.
There's yet another fixed issue regarding performance with large data - I found out that using OpenStruct is a performance killer so I changed them to ruby hashes (much much faster).
What we did not finish was the design - now we have two different kinds of pop-ups - alerts and rendered windows (which need more attention regarding styling). We need to hear some opinion and then finish this part (e.g. unify all pop-ups as rendered windows and get them designed by a real designer which is something I'd recommend rather than using ugly alerts).

@sodabrew

This comment has been minimized.

Show comment Hide comment
@sodabrew

sodabrew Dec 20, 2012

Owner

Ok, so there are at least four things going on ;) Let's leave it as-is then.

Are you entirely removing OpenStruct? I was wondering about what benefit it provides vs. Hashes. Maybe better behavior on older Ruby 1.8's? I don't have a sense of the risk for this speed benefit.

Re: popups: I'm generally a fan of the Rails 'flash' popups, they provide a consistent CSS interface to provide styling.

May I ask you to attach some cropped screenshots to this PR?

Owner

sodabrew commented Dec 20, 2012

Ok, so there are at least four things going on ;) Let's leave it as-is then.

Are you entirely removing OpenStruct? I was wondering about what benefit it provides vs. Hashes. Maybe better behavior on older Ruby 1.8's? I don't have a sense of the risk for this speed benefit.

Re: popups: I'm generally a fan of the Rails 'flash' popups, they provide a consistent CSS interface to provide styling.

May I ask you to attach some cropped screenshots to this PR?

@fhrbek

This comment has been minimized.

Show comment Hide comment
@fhrbek

fhrbek Dec 21, 2012

Contributor

OpenStruct: I have no idea if there is any other benefit than convenient calls of methods instead of addressing items by keys but it does not seem like too much added value to me. Basically I got rid of all of OpenStructs that were related to potentially large data in the model. You can check very easily if you browse changes for that commit (only OpenStruct refactoring and a single DB migration for indexing foreign keys are related to this ticket/commit).

Popups: What's a "Rails 'flash' popup"?

Here are examples of current (quite ugly) popups in the dashboard:

Alert Popup
Rendered Popup

Contributor

fhrbek commented Dec 21, 2012

OpenStruct: I have no idea if there is any other benefit than convenient calls of methods instead of addressing items by keys but it does not seem like too much added value to me. Basically I got rid of all of OpenStructs that were related to potentially large data in the model. You can check very easily if you browse changes for that commit (only OpenStruct refactoring and a single DB migration for indexing foreign keys are related to this ticket/commit).

Popups: What's a "Rails 'flash' popup"?

Here are examples of current (quite ugly) popups in the dashboard:

Alert Popup
Rendered Popup

@fhrbek

This comment has been minimized.

Show comment Hide comment
@fhrbek

fhrbek Dec 27, 2012

Contributor

Fixes for failing specs are now resubmitted as a new pull request:
puppetlabs#157

Contributor

fhrbek commented Dec 27, 2012

Fixes for failing specs are now resubmitted as a new pull request:
puppetlabs#157

@sodabrew

This comment has been minimized.

Show comment Hide comment
@sodabrew

sodabrew Jan 3, 2013

Owner

@fhrbek Next piece of low-hanging fruit to merge: let's pick a jquery-placeholder. I found many implementations, and just want to make sure we're picking the "best" one. Let's do a separate PR for just that change. My goal is to have the merge PR for class parameters be very focused on just this feature :)

http://code.google.com/p/jquery-placeholder-js/source/browse/trunk/jquery.placeholder.1.3.js
http://www.iliadraznin.com/2011/02/jquery-placeholder-plugin/
http://demo.frugalcoder.us/files/jquery.placeholder-1.1.9.js.txt
https://github.com/andrewrjones/jquery-placeholder-plugin
https://github.com/mathiasbynens/jquery-placeholder
https://github.com/danielstocks/jQuery-Placeholder

Owner

sodabrew commented Jan 3, 2013

@fhrbek Next piece of low-hanging fruit to merge: let's pick a jquery-placeholder. I found many implementations, and just want to make sure we're picking the "best" one. Let's do a separate PR for just that change. My goal is to have the merge PR for class parameters be very focused on just this feature :)

http://code.google.com/p/jquery-placeholder-js/source/browse/trunk/jquery.placeholder.1.3.js
http://www.iliadraznin.com/2011/02/jquery-placeholder-plugin/
http://demo.frugalcoder.us/files/jquery.placeholder-1.1.9.js.txt
https://github.com/andrewrjones/jquery-placeholder-plugin
https://github.com/mathiasbynens/jquery-placeholder
https://github.com/danielstocks/jQuery-Placeholder

@fhrbek

This comment has been minimized.

Show comment Hide comment
@fhrbek

fhrbek Jan 4, 2013

Contributor

The fruit does not hang so low as I thought :-(

I tried out all of suggested implementations and took notes...

Actually I found out that what I had done was wrong - as I replaced the original script with a different one it did not initialize itself (the original one did it automatically but these ones require an explicit initialization). That way it worked fine on browsers with native HTML5 support but did not work at all on old browsers (the placeholder text did not appear, otherwise it was fully functional).

So, when I tried these implementations including initialization and used placeholders in an ajaxified form, I found out that:

  1. http://code.google.com/p/jquery-placeholder-js/source/browse/trunk/jquery.placeholder.1.3.js

This is similar to the original script, it submits the placeholder value instead of an empty string even in HTML5 compliant browsers.

=> unusable

  1. http://www.iliadraznin.com/2011/02/jquery-placeholder-plugin/

This one submits the placeholder value instead of an empty string in non-HTML5 browsers

=> unusable

  1. http://demo.frugalcoder.us/files/jquery.placeholder-1.1.9.js.txt

Although technically the worst solution this is the only one that works fine in all tested situations.
The disadvantage of this solution is that it requires stylesheet tuning so that the absolute overlay of the input field would fit exactly in correct position and text font&size.

=> usable but ugly

  1. https://github.com/andrewrjones/jquery-placeholder-plugin

Same as #2

=> unusable

  1. https://github.com/mathiasbynens/jquery-placeholder

Same as #2

=> unusable

  1. https://github.com/danielstocks/jQuery-Placeholder

Same as #2

=> unusable

So, if we want to ajaxify the forms (which is a good thing IMHO) then the choice is narrowed to the ugly solution of #3 unless we find yet another solution or if we try to fix one of these.

Another option is to drop support for non-HTML5 browsers (I don't know how many potential users would be hit by it though).

Note: I was testing on 3 browsers:

HTML5: Chrome 23.0.1271.97, Firefox 17.0.1
non-HTML5: Microsoft Internet Explorer 8.0.6001.18702

That's all from me for today and I can continue this investigation on Monday.

Filip

Dne 4.1.2013 00:20, Aaron Stone napsal(a):

@fhrbek https://github.com/fhrbek Next piece of low-hanging fruit to merge: let's pick a jquery-placeholder. I found many implementations, and just want to make sure we're picking the "best" one. Let's do a separate PR for just that change. My goal is to have the merge PR for class parameters be very focused on just this feature :)

http://code.google.com/p/jquery-placeholder-js/source/browse/trunk/jquery.placeholder.1.3.js
http://www.iliadraznin.com/2011/02/jquery-placeholder-plugin/
http://demo.frugalcoder.us/files/jquery.placeholder-1.1.9.js.txt
https://github.com/andrewrjones/jquery-placeholder-plugin
https://github.com/mathiasbynens/jquery-placeholder
https://github.com/danielstocks/jQuery-Placeholder


Reply to this email directly or view it on GitHub puppetlabs#142 (comment).

Contributor

fhrbek commented Jan 4, 2013

The fruit does not hang so low as I thought :-(

I tried out all of suggested implementations and took notes...

Actually I found out that what I had done was wrong - as I replaced the original script with a different one it did not initialize itself (the original one did it automatically but these ones require an explicit initialization). That way it worked fine on browsers with native HTML5 support but did not work at all on old browsers (the placeholder text did not appear, otherwise it was fully functional).

So, when I tried these implementations including initialization and used placeholders in an ajaxified form, I found out that:

  1. http://code.google.com/p/jquery-placeholder-js/source/browse/trunk/jquery.placeholder.1.3.js

This is similar to the original script, it submits the placeholder value instead of an empty string even in HTML5 compliant browsers.

=> unusable

  1. http://www.iliadraznin.com/2011/02/jquery-placeholder-plugin/

This one submits the placeholder value instead of an empty string in non-HTML5 browsers

=> unusable

  1. http://demo.frugalcoder.us/files/jquery.placeholder-1.1.9.js.txt

Although technically the worst solution this is the only one that works fine in all tested situations.
The disadvantage of this solution is that it requires stylesheet tuning so that the absolute overlay of the input field would fit exactly in correct position and text font&size.

=> usable but ugly

  1. https://github.com/andrewrjones/jquery-placeholder-plugin

Same as #2

=> unusable

  1. https://github.com/mathiasbynens/jquery-placeholder

Same as #2

=> unusable

  1. https://github.com/danielstocks/jQuery-Placeholder

Same as #2

=> unusable

So, if we want to ajaxify the forms (which is a good thing IMHO) then the choice is narrowed to the ugly solution of #3 unless we find yet another solution or if we try to fix one of these.

Another option is to drop support for non-HTML5 browsers (I don't know how many potential users would be hit by it though).

Note: I was testing on 3 browsers:

HTML5: Chrome 23.0.1271.97, Firefox 17.0.1
non-HTML5: Microsoft Internet Explorer 8.0.6001.18702

That's all from me for today and I can continue this investigation on Monday.

Filip

Dne 4.1.2013 00:20, Aaron Stone napsal(a):

@fhrbek https://github.com/fhrbek Next piece of low-hanging fruit to merge: let's pick a jquery-placeholder. I found many implementations, and just want to make sure we're picking the "best" one. Let's do a separate PR for just that change. My goal is to have the merge PR for class parameters be very focused on just this feature :)

http://code.google.com/p/jquery-placeholder-js/source/browse/trunk/jquery.placeholder.1.3.js
http://www.iliadraznin.com/2011/02/jquery-placeholder-plugin/
http://demo.frugalcoder.us/files/jquery.placeholder-1.1.9.js.txt
https://github.com/andrewrjones/jquery-placeholder-plugin
https://github.com/mathiasbynens/jquery-placeholder
https://github.com/danielstocks/jQuery-Placeholder


Reply to this email directly or view it on GitHub puppetlabs#142 (comment).

@sodabrew

This comment has been minimized.

Show comment Hide comment
@sodabrew

sodabrew Jan 5, 2013

Owner

Huge thank you for the thorough review of these options!!

For the options that require initialization, we can put a self-initializing piece of JS at the bottom of the script file. For the one that requires CSS changes, that's less attractive. Maybe we can adjust the script to use our existing CSS? I'll take a look at this over the weekend or next week.

Owner

sodabrew commented Jan 5, 2013

Huge thank you for the thorough review of these options!!

For the options that require initialization, we can put a self-initializing piece of JS at the bottom of the script file. For the one that requires CSS changes, that's less attractive. Maybe we can adjust the script to use our existing CSS? I'll take a look at this over the weekend or next week.

@fhrbek

This comment has been minimized.

Show comment Hide comment
@fhrbek

fhrbek Jan 7, 2013

Contributor

One more thing to investigate - we used prototype's ajax in our implementation in the class_parameters branch. I noticed that you got rid of this library in your rails3 branch. Perhaps replacing prototype's ajax calls with jquery's ajax calls would be compatible with jquery placeholder implementations. I can look into it if you like.

Contributor

fhrbek commented Jan 7, 2013

One more thing to investigate - we used prototype's ajax in our implementation in the class_parameters branch. I noticed that you got rid of this library in your rails3 branch. Perhaps replacing prototype's ajax calls with jquery's ajax calls would be compatible with jquery placeholder implementations. I can look into it if you like.

@sodabrew

This comment has been minimized.

Show comment Hide comment
@sodabrew

sodabrew Jan 7, 2013

Owner

Yes, using jQuery's ajax would be better.

By the way, could you rebase your branch once more off current master? You should be able to drop the spec fixes commit out of the way, and also split out the placeholder commit as a separate PR.

Owner

sodabrew commented Jan 7, 2013

Yes, using jQuery's ajax would be better.

By the way, could you rebase your branch once more off current master? You should be able to drop the spec fixes commit out of the way, and also split out the placeholder commit as a separate PR.

@sodabrew

This comment has been minimized.

Show comment Hide comment
@sodabrew

sodabrew Jan 29, 2013

Owner

Definitely time for a rebase to master! :)

Owner

sodabrew commented Jan 29, 2013

Definitely time for a rebase to master! :)

@sodabrew

This comment has been minimized.

Show comment Hide comment
@sodabrew

sodabrew Feb 12, 2013

Owner

@fhrbek please rebase from master when you have some time! :)

Owner

sodabrew commented Feb 12, 2013

@fhrbek please rebase from master when you have some time! :)

@fhrbek

This comment has been minimized.

Show comment Hide comment
@fhrbek

fhrbek Feb 14, 2013

Contributor

Yeah, when I have some time ;-)

Quite busy these days but I'll find some time for it soon.

Filip

Dne 12.2.2013 21:26, Aaron Stone napsal(a):

@fhrbek https://github.com/fhrbek please rebase from master when you have some time! :)


Reply to this email directly or view it on GitHub puppetlabs#142 (comment).

Contributor

fhrbek commented Feb 14, 2013

Yeah, when I have some time ;-)

Quite busy these days but I'll find some time for it soon.

Filip

Dne 12.2.2013 21:26, Aaron Stone napsal(a):

@fhrbek https://github.com/fhrbek please rebase from master when you have some time! :)


Reply to this email directly or view it on GitHub puppetlabs#142 (comment).

@stahnma

This comment has been minimized.

Show comment Hide comment
@stahnma

stahnma Mar 6, 2013

Contributor

I theoretically love this pull request. Fillip, I hope you get some time very soon to rebase. This would be an awesome addition to Dashboard 1.3. @khussey -- wanted to add this back on your radar.

Contributor

stahnma commented Mar 6, 2013

I theoretically love this pull request. Fillip, I hope you get some time very soon to rebase. This would be an awesome addition to Dashboard 1.3. @khussey -- wanted to add this back on your radar.

@puppetcla

This comment has been minimized.

Show comment Hide comment
@puppetcla

puppetcla Mar 21, 2013

CLA Signed by fhrbek on 2013-01-07 21:00:00 -0800

CLA Signed by fhrbek on 2013-01-07 21:00:00 -0800

@sodabrew sodabrew referenced this pull request Apr 11, 2013

Merged

Class Parameters #198

@sodabrew sodabrew closed this Apr 12, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment