Bug 238994 - Review Request: memcached - High Performance, Distributed Memory Object Cache
Review Request: memcached - High Performance, Distributed Memory Object Cache
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Mamoru TASAKA
Fedora Package Reviews List
:
: 243666 (view as bug list)
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-05-04 07:32 EDT by Paul Lindner
Modified: 2007-11-30 17:12 EST (History)
7 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-07-20 10:22:39 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
mtasaka: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)
fixes 'unexpected success' in maxconnect.t (840 bytes, patch)
2007-05-08 04:26 EDT, Bernard Johnson
no flags Details | Diff

  None (edit)
Description Paul Lindner 2007-05-04 07:32:38 EDT
Spec URL: http://www.inuus.com/memcached.spec
SRPM URL: http://www.inuus.com/memcached-1.2.2-1.src.rpm
Description: 

memcached is a high-performance, distributed memory object cachingsystem, generic in nature, but intended for use in speeding up dynamicweb applications by alleviating database load.

Available rpmbuild rebuild options :  
   --with=threads   - build a multiprocessor optimized memcached server

** Note -- need a sponsor, this is my first package for Fedora extras.  Also note that this srpm contains items that will be committed to the 1.2.3 version of memcached.  When we iron out the spec file I'll commit it to the upstream memcached tree and release 1.2.3.
Comment 1 Ruben Kerkhof 2007-05-05 07:10:03 EDT
Hi Paul,

I'm not a sponsor, but will give you some initial comments:

- Perl is on the exception list, so no need to Require it (wiki: PackagingGuidelines#Exceptions)
- Since you have a BuildRequires on libevent-devel, rpm creates a Requires: libevent, so there's no need 
to specify that.
- I would suggest building with --with=threads by default, most servers are multiproc nowadays. What 
do you think?
- At least drop the --with=threads part from the description. It's not very for the end-user.
- Replace %{_sysconfdir}/rc.d/init.d with %{_initrddir}
- Be consistent with $RPM_BUILD_ROOT or %{buildroot}, use one or the other, not both
- Replace $RPM_BUILD_DIR/%{name}-%{version}/scripts/memcached.sysv with scripts/memcached.sysv 
in the %install section, there's no need to use $RPM_BUILD_DIR
Comment 2 Paul Lindner 2007-05-07 17:51:32 EDT
Thanks Ruben.

I've applied your suggestions, including going with threads in the default build.

New SRPM is at http://www.inuus.com/memcached-1.2.2-2.src.rpm

updated spec is in the location specified above or in

 http://code.sixapart.com/svn/memcached/trunk/server/memcached.spec
Comment 3 Bernard Johnson 2007-05-08 04:26:34 EDT
Created attachment 154318 [details]
fixes 'unexpected success' in maxconnect.t

I'm not a sponsor either, but here are some more comments:

1) Package should Require initscripts

2) I suggest you turn on the make test portion (we like automated testing).  To
do this, you need to BuildRequire perl(Test::More).  Then to make things sane
in the test, you'll want to:

# delete test that weren't written, and daemonize doesn't work in koji
rm -f
t/{binary-get,daemonize,lru,managed-buckets,multiversioning,slab-reassign,unixsocket}.t


then apply the attached patch to fix one test that wasn't completed.

(By the way, daemonize can be fixed by using a random port for testing)

I also see that memcache-debug gets installed?	Is that useful?
Comment 4 Paul Lindner 2007-05-10 17:36:17 EDT
Thanks Bernard!

Suggestions incorporated into 1.2.2-3

http://www.inuus.com/memcached-1.2.2-3.src.rpm
http://www.inuus.com/memcached.spec
Comment 5 Paul Lindner 2007-05-12 07:13:21 EDT
A few small changes -- more unit tests added upstream, removed tabs from spec file.

rpmlint now runs clean on src and i386 rpms...


http://www.inuus.com/memcached-1.2.2-4.src.rpm
http://www.inuus.com/memcached.spec


Not sure if there's much more I can do..  I believe this spec file should be
good to go..
Comment 6 Roy-Magne Mo 2007-05-12 08:22:16 EDT
How about disabling sendfile for now?
http://projects.linpro.no/pipermail/varnish-misc/2007-May/000412.html
Comment 7 Roy-Magne Mo 2007-05-12 08:58:10 EDT
(In reply to comment #6)
> How about disabling sendfile for now?
> http://projects.linpro.no/pipermail/varnish-misc/2007-May/000412.html

sorry about that one, wrongz bz :( 
Comment 8 Mamoru TASAKA 2007-05-23 12:46:36 EDT
Well, I have not checked this in detail, however
-4 seems good. I will review this fully later
(however as I live in Japan (UTC + 9h), I once go to
 sleep...)

Some questions:
? Would you explain why you removed memcached-debug?

As this is a sponsor-needed ticket:
-------------------------------------------------------------
NOTE: Before being sponsored:

This package will be accepted with another few work. 
But before I accept this package, someone (I am a candidate) 
must sponsor you.

Once you are sponsored, you have the right to review other 
submitters' review requests and approve the packages formally. 
For this reason, the person who want to be sponsored (like you) 
are required to "show that you have an understanding 
of the process and of the packaging guidelines" as is described
on :
http://fedoraproject.org/wiki/PackageMaintainers/HowToGetSponsored

Usually there are two ways to show this.
A. submit other review requests with enough quality.
B. Do a "pre-review" of other person's review request
   (at the time you are not sponsored, you cannot do
   a formal review)

When you have submitted a new review request or have pre-reviewed other 
person's review request, please write the bug number on this bug report 
so that I can check your comments or review request.

Fedora Extras package review requests which are waiting for someone to
review can be checked on:
https://bugzilla.redhat.com/bugzilla/buglist.cgi?cmdtype=runnamed&namedcmd=mtasaka-review-noone

Review guidelines are described mainly on:
http://fedoraproject.org/wiki/Packaging/ReviewGuidelines
http://fedoraproject.org/wiki/Packaging/Guidelines
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets
------------------------------------------------------------
Comment 9 Paul Lindner 2007-05-24 10:56:47 EDT
Tasaka-san: 

memcached-debug is of interest to developers working on the memcached code base.
 Thus it was removed from the general RPM.  The people who need this will also
need to source to work with it.  The -v flags are sufficient for end-users.

Thanks!
Comment 10 Mamoru TASAKA 2007-05-24 11:40:53 EDT
Well, one critical issue:

* Source differ:

----------------------------------------------------------
[tasaka1@localhost memcached]$ ls -la */*gz
-rw------- 1 tasaka1 tasaka1 166201 2007-05-03 08:22
downloaded/memcached-1.2.2.tar.gz
-rw-r--r-- 1 tasaka1 tasaka1 177272 2007-05-12 19:58
memcached-1.2.2-4.fc7/memcached-1.2.2.tar.gz
[tasaka1@localhost memcached]$ md5sum */*gz
a08851f7fa7b15e92ee6320b7a79c321  downloaded/memcached-1.2.2.tar.gz
5e1e7ee5a3834e7b600e722389b7f385  memcached-1.2.2-4.fc7/memcached-1.2.2.tar.gz
------------------------------------------------------------
- The tarball you included in your srpm differs from
  which I downloaded from the URL specified by your spec file
  as Source0.
  Did you apply a patch locally and repackage? If so, please
  don't do it and anyway use upstream tarball.


Comment 11 Paul Lindner 2007-05-24 13:19:04 EDT
Okay then.  I have upstream control.  I'll 1.23 pushed out and repackage.

I was just hoping I could get consensus on the spec file so I could ship that
with memcache itself.

thanks.
Comment 12 Mamoru TASAKA 2007-05-24 13:54:22 EDT
(In reply to comment #11)
> I was just hoping I could get consensus on the spec file so I could ship that
> with memcache itself.

Well, some upstream tends to release new tarball during
review request every time the spec file is modified, however
it ignores people not using rpm packaging manager.

So if you want to fix just spec file only during review request,
please fix the spec file for Fedora only and don't change
upstream tarball unless some other issues to be fixed are found.
Comment 13 Ruben Kerkhof 2007-05-25 01:38:31 EDT
(in reply to comment #3)
Require: initscripts is not really necessary. It's a base package, and on http://fedoraproject.org/wiki/
Extras/FullExceptionList
Comment 14 Matthias Saou 2007-05-29 11:40:36 EDT
A few comments :
- You could be a little more consistent in your spec wrt "%{name}" vs.
"memcached" usage. For instance, the init script is included as "memcached" but
the scriplets control the service as "%{name}".
- Would you consider replacing the current empty OPTIONS with "-l 127.0.0.1" in
order to "secure" default installations?
- As already mentioned the "Requires: initscripts" is useless since
"Requires(preun): /sbin/chkconfig" already makes sure it'll be installed.
Comment 15 Mamoru TASAKA 2007-06-05 11:39:01 EDT
ping?
Comment 16 Mamoru TASAKA 2007-06-12 11:24:31 EDT
ping again?
Comment 17 Mamoru TASAKA 2007-06-12 22:58:14 EDT
*** Bug 243666 has been marked as a duplicate of this bug. ***
Comment 18 Mamoru TASAKA 2007-06-20 11:52:00 EDT
Again ping?
I will close this bug if no response from the submitter is
received by 7/1.
Comment 19 Paul Lindner 2007-06-20 12:27:41 EDT
1.2.3 is almost ready for release.

A new package based on that will be available for review then.
Comment 20 Mamoru TASAKA 2007-06-20 12:33:42 EDT
Okay. Thank you for your information.
Comment 21 Matthias Saou 2007-06-20 14:01:13 EDT
If Paul is more interested in upstream development, I'd be more than happy to
take over the package submission/maintainership, or simply become co-maintainer,
as I use memcached quite extensively already ;-)
Comment 22 Konstantin Ryabitsev 2007-07-06 13:51:40 EDT
Same here. What can we do to make this hit Fedora and EPEL?
Comment 23 Mamoru TASAKA 2007-07-06 14:00:11 EDT
(In reply to comment #22)
> Same here. 
What do you mean by this?

> What can we do to make this hit Fedora and EPEL?
Someone must create new spec/srpm and must have it pass
other people's review.
So currently who is going to maintain this on Fedora?
Comment 24 Paul Lindner 2007-07-06 14:07:09 EDT
Brad Fitzpatrick will roll out memcached 1.23.  I will update the spec/srpm as
soon as that is done.

ETA sometime between now and the monday memcached hackathon.
Comment 25 Ruben Kerkhof 2007-07-06 15:06:20 EDT
I'm interested in co-maintaining this as well. I'm already maintaining Perlbal and MogileFS, the other 
products from the LiveJournal guys.
Comment 26 Paul Lindner 2007-07-06 15:45:55 EDT
Updated RPM/spec now available

Spec URL: http://www.inuus.com/memcached.spec
SRPM URL: http://www.inuus.com/memcached-1.2.3-1.src.rpm

Comment 27 Mamoru TASAKA 2007-07-06 21:31:12 EDT
The requested URL /memcached-1.2.3-1.src.rpm was not found on this server.
Comment 28 Paul Lindner 2007-07-06 21:47:32 EDT
source rpm is now at the correct URL, or you can grab it at 

http://www.inuus.com/memcached-1.2.3-1.fc7.src.rpm

Apologies for the confusion.

Regards,
Paul
Comment 29 Paul Lindner 2007-07-06 21:50:31 EDT
(In reply to comment #25)
> I'm interested in co-maintaining this as well. I'm already maintaining Perlbal
and MogileFS, the other 
> products from the LiveJournal guys.

Matthias, Ruben, 

I'm more than happy to work together with others to get this packaged and complete.

Regards,
Paul
Comment 30 Mamoru TASAKA 2007-07-07 03:09:00 EDT
For 1.2.3-1: Almost okay.

* Timestamps
  - To keep timestamp on man file, please use the following.
-----------------------------------------------------------
make install DESTDIR=%{buildroot} INSTALL="%{__install} -p"
-----------------------------------------------------------

* Changelog
  - rpm EVR is 1.2.3-1, however the last entry of %changelog
    is 1.2.2-5.

! (Request: NOT a blocker)
  - Initscripts
    We (including me) are now required to fix initscripts
    to match LSB requirement. Check:
    http://fedoraproject.org/wiki/FCNewInit/Initscripts
    For now, I found:
    * force-reload option is missing
    * starting memcached whe memcached is already running
      fails:
-----------------------------------------------------------
[root@localhost ~]# LANG=C service memcached start ; echo $?
Starting memcached:                                        [  OK  ]
0
[root@localhost ~]# LANG=C service memcached status ; echo $?
memcached (pid 5742) is running...
0
[root@localhost ~]# LANG=C service memcached start ; echo $?
Starting memcached: bind(): Address already in use
failed to listen
                                                           [FAILED]
1
------------------------------------------------------------
    * Non-impremented option returns 1, not 3 (well, I guess
      most of current initscripts return 1 for now...)
------------------------------------------------------------
[root@localhost ~]# LANG=C service memcached foo ; echo $?
Usage: /etc/init.d/memcached {start|stop|status|restart|reload|condrestart}
1
------------------------------------------------------------
     Please fix above, for example (but I WON'T treat these
     as a blocker).

And.. as this is NEEDSPONSOR ticket, I rewrite for confirmation:
-------------------------------------------------------------
NOTE: Before being sponsored:

This package will be accepted with another few work. 
But before I accept this package, someone (I am a candidate) 
must sponsor you.

Once you are sponsored, you have the right to review other 
submitters' review requests and approve the packages formally. 
For this reason, the person who want to be sponsored (like you) 
are required to "show that you have an understanding 
of the process and of the packaging guidelines" as is described
on :
http://fedoraproject.org/wiki/PackageMaintainers/HowToGetSponsored

Usually there are two ways to show this.
A. submit other review requests with enough quality.
B. Do a "pre-review" of other person's review request
   (at the time you are not sponsored, you cannot do
   a formal review)

When you have submitted a new review request or have pre-reviewed other 
person's review request, please write the bug number on this bug report 
so that I can check your comments or review request.

Fedora package collection review requests which are waiting for someone to
review can be checked on:
http://fedoraproject.org/PackageReviewStatus/NEW.html
(NOTE: please don't choose "Merge Review")


Review guidelines are described mainly on:
http://fedoraproject.org/wiki/Packaging/ReviewGuidelines
http://fedoraproject.org/wiki/Packaging/Guidelines
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets
------------------------------------------------------------
Comment 31 Paul Lindner 2007-07-07 10:58:38 EDT
1.2.3-2

Added changelog entries pointing at 1.2.3-2

Adjusted init scripts to be more LSB compliant.  Found that LSB's start_daemon
does not support the -p flag so we're still using daemon with --pidfile args.

Adjusted make install to preserve timestamp.

Here's the latest:

http://www.inuus.com/memcached.spec
http://www.inuus.com/memcached-1.2.3-2.fc7.src.rpm
Comment 32 Konstantin Ryabitsev 2007-07-07 11:58:24 EDT
Should it be running as user nobody? I have vague recollection of the rule that
each process should be running as its own user.
Comment 33 Mamoru TASAKA 2007-07-07 12:04:40 EDT
Well, memcached itself is okay. Then I will wait for your
pre-review or another review request.

(In reply to comment #32)
> Should it be running as user nobody? 
It may well that memcached may create its unique user/group,
but I regard it as NOT a blocker.
Comment 34 Paul Lindner 2007-07-09 09:30:47 EDT
Starting reviews of other packages, completed 246747 - postgresql-ip4r
Comment 35 Paul Lindner 2007-07-09 09:39:52 EDT
(In reply to comment #32)
> Should it be running as user nobody? I have vague recollection of the rule that
> each process should be running as its own user.

I'm just starting to read:

http://fedoraproject.org/wiki/PackageUserCreation
http://fedoraproject.org/wiki/PackageUserRegistry

These changes appear more involved, and would require some time to get resolved.

If possible I'd like to release with memcached running as user nobody for the
moment, and then follow the correct process for adding users and registering a
memcached user.
Comment 36 Mamoru TASAKA 2007-07-09 09:58:37 EDT
(In reply to comment #34)
> Starting reviews of other packages, completed 246747 - postgresql-ip4r
Well, I checked your pre-review and it seems good for
initial comments.


(In reply to comment #35)
> (In reply to comment #32)
> > Should it be running as user nobody? I have vague recollection of the rule that
> > each process should be running as its own user.
> 
> I'm just starting to read:
> 
> http://fedoraproject.org/wiki/PackageUserCreation
> http://fedoraproject.org/wiki/PackageUserRegistry
> 
> These changes appear more involved, and would require 
> some time to get resolved.
I don't recommend fedora-usermgmt (and I don't request you
to use it or not to use it...)

Okay.
------------------------------------------------------
    This package (memcached) is APPROVED by me
------------------------------------------------------

Please follow the procedure written on
http://fedoraproject.org/wiki/PackageMaintainers/Join
from "Get a Fedora Account"

If you want to push this package also on F-7, you
also have to check:
http://fedoraproject.org/wiki/Infrastructure/UpdatesSystem/Bodhi-info-DRAFT
after the URL above.

Matthias and Ruben, do you want to co-maintain this package?
Comment 37 Paul Lindner 2007-07-09 14:15:59 EDT
Okay, getting closer.  I've had my fedora account 'plindner' for quite a while now.

I tried following the instructions here:

http://fedoraproject.org/wiki/PackageMaintainers/CVSAdminProcedure

but it appears that I do now have the proper authority to do that yet.

regards
Comment 38 Jason Tibbitts 2007-07-09 14:27:58 EDT
According to the account system you have completed the CLA but haven't even
applied for cvsextras access yet.  See:
  http://fedoraproject.org/wiki/PackageMaintainers/Join
specifically the "Get Sponsored" step.
Comment 39 Mamoru TASAKA 2007-07-09 14:38:58 EDT
Now I am sponsoring you.
Comment 40 Paul Lindner 2007-07-09 23:22:47 EDT
New Package CVS Request
=======================
Package Name: memcached
Short Description: High Performance, Distributed Memory Object Cache
Owners: lindner@inuus.com
Branches: FC-6 F-7
InitialCC: lindner@inuus.com

Comment 41 Paul Lindner 2007-07-09 23:24:35 EDT
argh... can't set fedora-cvs flag yet...  hopefully fedorabugs permission gets
propagated...

Comment 42 Paul Lindner 2007-07-10 11:32:03 EDT
New Package CVS Request
=======================
Package Name: memcached
Short Description: High Performance, Distributed Memory Object Cache
Owners: lindner@inuus.com
Branches: FC-6 F-7 EL-4 EL-5
InitialCC: lindner@inuus.com

(adding EL-4 and EL-5 branches while we're waiting for cvs approval.)
Comment 43 Kevin Fenzi 2007-07-10 12:14:54 EDT
cvs done.
Comment 44 Matthias Saou 2007-07-10 23:49:40 EDT
(In reply to comment #36)
> Matthias and Ruben, do you want to co-maintain this package?

I would definitely be interested in doing so!
Comment 45 Paul Lindner 2007-07-11 09:18:11 EDT
Okay, here's the modification request.  For some reason I still do not have
access rights to modify the fedora-cvs flag.  Could someone with privileges do that?

Thanks


Package Change Request
======================
Package Name: memcached
Updated Owners: lindner@inuus.com,matthias@rpmforge.net
Updated CC: lindner@inuus.com,matthias@rpmforge.net
Comment 46 Ruben Kerkhof 2007-07-11 13:44:26 EDT
Hey Paul, so am I.
Comment 47 Paul Lindner 2007-07-11 14:49:12 EDT
Amended change request:

Package Change Request
======================
Package Name: memcached
Updated Owners: lindner@inuus.com,matthias@rpmforge.net,ruben@rubenkerkhof.com
Updated CC: lindner@inuus.com,matthias@rpmforge.net,ruben@rubenkerkhof.com

Thanks!
Comment 48 Kevin Fenzi 2007-07-11 22:19:26 EDT
cvs done.
Comment 49 Paul Lindner 2007-07-13 09:44:41 EDT
Okay, we have a build in devel for F8.

Please test it if you can once the RPM hits the rawhide mirrors.

I'm guessing that once we test it out we can mark this bug as NEXTRELEASE?

And then, on to F-7 and EPEL....

Thanks!
Comment 50 Mamoru TASAKA 2007-07-15 11:10:13 EDT
(In reply to comment #49)
> I'm guessing that once we test it out we can mark this bug as NEXTRELEASE?

Please close this bug as NEXTRELEASE once you rebuilt this.
Comment 51 Mamoru TASAKA 2007-07-20 08:12:42 EDT
Again, please close this bug when rebuild is done.
Comment 52 Paul Lindner 2007-07-20 10:22:39 EDT
Rebuild is done.  Looks good for Fedora 8.
Comment 53 Konstantin Ryabitsev 2007-07-23 15:36:45 EDT
Could you please build EPEL branches as well?
Comment 54 Paul Lindner 2007-07-25 11:24:30 EDT
EPEL for EL-5 coming, also FC6.

EL-4 not yet, since it seems that libevent-devel isn't available in that repo....

I'll investigate...


Note You need to log in before you can comment on or make changes to this bug.