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

Re-add --thin to fix #20072 and rhbz#1560978 #395

Merged
merged 1 commit into from Oct 31, 2018

Conversation

Projects
None yet
6 participants
@fcami
Copy link
Contributor

fcami commented Oct 31, 2018

Reverts #20754 to bring back #20072 ( https://projects.theforeman.org/issues/20072 )
Fixes rhbz#1560978 ( https://bugzilla.redhat.com/show_bug.cgi?id=1560978 )

Note that I did not test with a large DB but --thin seems to be indeed slightly faster, which could make a difference for 1K+ hosts.

# time hammer --csv host list --organization rht --location rdu
Id,Name,Operating System,Host Group,IP,MAC,Content View,Lifecycle Environment
2,test.example.org,RedHat 7.6,emptyHG,,00:00:00:00:00:00,cv7,dev

real	0m1.739s
user	0m1.295s
sys	0m0.167s
# time hammer --csv host list --organization rht --location rdu --thin 1
Id,Name,Operating System,Host Group,IP,MAC,Content View,Lifecycle Environment
2,test.example.org,"","",,,,

real	0m1.623s
user	0m1.306s
sys	0m0.150s
@theforeman-bot

This comment has been minimized.

Copy link
Member

theforeman-bot commented Oct 31, 2018

Do not merge! This patch has not been tested yet.

Can an existing organization member please verify this patch?

@tstrachota

This comment has been minimized.

Copy link
Member

tstrachota commented Oct 31, 2018

[test]

Fixes #25349 - Re-add --thin
Reverts #20754 to bring back #20072
Fixes rhbz#1560978

Signed-Off-By: François Cami <fcami@fedoraproject.org>

@fcami fcami force-pushed the fcami:bring-thin-back branch from 120533e to 4639cc6 Oct 31, 2018

@theforeman-bot

This comment has been minimized.

Copy link
Member

theforeman-bot commented Oct 31, 2018

@fcami, the Redmine ticket used is for a different project than the one associated with this GitHub repository. Please either:

If changing the ticket number used, remember to update the PR title and the commit message (using git commit --amend).


This message was auto-generated by Foreman's prprocessor

@iNecas

This comment has been minimized.

Copy link
Member

iNecas commented Oct 31, 2018

ok to test

@iNecas

This comment has been minimized.

Copy link
Member

iNecas commented Oct 31, 2018

I've tested this and with my setup (several thousands of hosts), the difference is significant:

bundle exec bin/hammer host list --thin=yes 
2.76s user 0.27s system 65% cpu 4.638 total

bundle exec bin/hammer host list
20.55s user 1.36s system 12% cpu 2:53.35 total
@iNecas

iNecas approved these changes Oct 31, 2018

Copy link
Member

iNecas left a comment

ACK: tested and works well. @tstrachota ok to merge?

@tstrachota tstrachota merged commit 7ab5eed into theforeman:master Oct 31, 2018

1 check passed

hammer Build finished. 4014 tests run, 0 skipped, 0 failed.
Details
@tstrachota

This comment has been minimized.

Copy link
Member

tstrachota commented Oct 31, 2018

Merged, thanks @fcami and @iNecas for testing that!

@tbrisker

This comment has been minimized.

Copy link
Member

tbrisker commented Nov 1, 2018

Should this also be pulled in to the latest stable branch?

@fcami

This comment has been minimized.

Copy link
Contributor Author

fcami commented Nov 1, 2018

@tbrisker Considering it's a regression I'd say yes please but I'll defer to @tstrachota and @iNecas

@mbacovsky

This comment has been minimized.

Copy link
Member

mbacovsky commented Nov 1, 2018

@tbrisker I have some more PRs to add to the stable branch, aiming for the next weeks build. I'll include this one too.

mbacovsky added a commit that referenced this pull request Nov 6, 2018

Fixes #25349 - Re-add --thin into host list (#395)
Reverts #20754 to bring back #20072
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.