Front page

[PATCH 2/3] ForceLock: new subcommand naming, add lock-force-release-client

1113f1af92984443b3e44700449aadc1
ATHENS BARBECUE UNWIND

From: "Robin H. Johnson" <robbat2@gentoo.org>
Date: Sat, 18 Jun 2016 02:48:42 -0700

   Reduce code duplication by introducing a repo context manager per
   PEP-343. This paves the way to introduce more lock handling functions
   without adding duplicate code.
   
   Signed-off-by: Robin H. Johnson <robbat2@gentoo.org>
   ---
    obnamlib/plugins/force_lock_plugin.py | 62 ++++++++++++++++-------------------
    1 file changed, 28 insertions(+), 34 deletions(-)
   
   diff --git a/obnamlib/plugins/force_lock_plugin.py b/obnamlib/plugins/force_lock_plugin.py
   index 85995be..28bf641 100644
   --- a/obnamlib/plugins/force_lock_plugin.py
   +++ b/obnamlib/plugins/force_lock_plugin.py
   @@ -24,6 +24,24 @@ class RepositoryAccessError(obnamlib.ObnamError):
        msg = 'Repository does not exist or cannot be accessed: {error}'
    
    
   +class RepoAccessWrapper(object):
   +    def __init__(self, plugin):
   +        self.plugin = plugin
   +        self.repo = None
   +
   +    def __enter__(self):
   +        self.plugin.app.settings.require('repository')
   +        repourl = self.plugin.app.settings['repository']
   +        logging.info('Repository: %s', repourl)
   +        try:
   +            self.repo = self.plugin.app.get_repository_object()
   +        except OSError, e:
   +            raise RepositoryAccessError(error=str(e))
   +        return self.repo
   +
   +    def __exit__(self, type, value, traceback):
   +        self.repo.close()
   +
    class ForceLockPlugin(obnamlib.ObnamPlugin):
    
        def enable(self):
   @@ -33,25 +51,12 @@ class ForceLockPlugin(obnamlib.ObnamPlugin):
        def force_lock(self, args):
            '''Force a locked repository to be open.'''
    
   -        self.app.settings.require('repository')
   -
   -        repourl = self.app.settings['repository']
   -        client_name = self.app.settings['client-name']
            logging.info('Forcing lock')
   -        logging.info('Repository: %s', repourl)
   -
   -        try:
   -            repo = self.app.get_repository_object()
   -        except OSError, e:
   -            raise RepositoryAccessError(error=str(e))
   -
   -        repo.force_client_list_lock()
   -        for client_name in repo.get_client_names():
   -            repo.force_client_lock(client_name)
   -        repo.force_chunk_indexes_lock()
   -
   -        repo.close()
   -
   +        with RepoAccessWrapper(self) as repo:
   +            repo.force_client_list_lock()
   +            for client_name in repo.get_client_names():
   +                repo.force_client_lock(client_name)
   +            repo.force_chunk_indexes_lock()
            return 0
    
        def lock(self, args):
   @@ -61,24 +66,13 @@ class ForceLockPlugin(obnamlib.ObnamPlugin):
    
            '''
    
   -        self.app.settings.require('repository')
   -
   -        repourl = self.app.settings['repository']
            client_name = self.app.settings['client-name']
            logging.info('Creating lock')
   -        logging.info('Repository: %s', repourl)
            logging.info('Client: %s', client_name)
   -
   -        try:
   -            repo = self.app.get_repository_object()
   -        except OSError, e:
   -            raise RepositoryAccessError(error=str(e))
   -
   -        repo.lock_client_list()
   -        if client_name:
   -            repo.lock_client(client_name)
   -        repo.lock_chunk_indexes()
   -
   -        repo.close()
   +        with RepoAccessWrapper(self) as repo:
   +            repo.lock_client_list()
   +            if client_name:
   +                repo.lock_client(client_name)
   +            repo.lock_chunk_indexes()
    
            return 0
From: "Robin H. Johnson" <robbat2@gentoo.org>
Date: Sat, 18 Jun 2016 02:48:43 -0700

   The force-lock command name is ambigious with the addition of new
   lock-handling subcommands.
   
   Convert to a new naming scheme with the addition of a command to
   forcibly release the lock for a single client.
   
   This command is useful if a single client exits because it could not
   acquire the global locks, and you only want to release that client's
   lock to resume it.
   
   Print a deprecation message for any old users of force-lock, and do not
   include force-lock in the help output.
   
   Signed-off-by: Robin H. Johnson <robbat2@gentoo.org>
   ---
    obnamlib/plugins/force_lock_plugin.py | 27 ++++++++++++++++++++++++---
    1 file changed, 24 insertions(+), 3 deletions(-)
   
   diff --git a/obnamlib/plugins/force_lock_plugin.py b/obnamlib/plugins/force_lock_plugin.py
   index 28bf641..a14d680 100644
   --- a/obnamlib/plugins/force_lock_plugin.py
   +++ b/obnamlib/plugins/force_lock_plugin.py
   @@ -45,13 +45,24 @@ class RepoAccessWrapper(object):
    class ForceLockPlugin(obnamlib.ObnamPlugin):
    
        def enable(self):
   -        self.app.add_subcommand('force-lock', self.force_lock)
   +        # legacy:
   +        self.app.add_subcommand('force-lock', self.force_lock, hidden=True)
            self.app.add_subcommand('_lock', self.lock, hidden=True)
    
   +        # new:
   +        self.app.add_subcommand('lock-force-release-all', self.force_release_all_locks)
   +        self.app.add_subcommand('lock-force-release-client', self.force_release_client_lock)
   +
        def force_lock(self, args):
            '''Force a locked repository to be open.'''
   -
   -        logging.info('Forcing lock')
   +        self.app.ts.notify("force-lock is deprecated.  " +
   +                           "Use force-release-all-locks or a " +
   +                           "more specific variant)")
   +        self.force_release_all_locks(args)
   +
   +    def force_release_all_locks(self, args):
   +        '''Forcibly release all locks.'''
   +        logging.info('Forcibly releasing all locks')
            with RepoAccessWrapper(self) as repo:
                repo.force_client_list_lock()
                for client_name in repo.get_client_names():
   @@ -59,6 +70,16 @@ class ForceLockPlugin(obnamlib.ObnamPlugin):
                repo.force_chunk_indexes_lock()
            return 0
    
   +    def force_release_client_lock(self, args):
   +        '''Forcibily release a specific client lock.'''
   +
   +        client_name = self.app.settings['client-name']
   +        logging.info('Forcibly releasing client lock for %s', client_name)
   +        with RepoAccessWrapper(self) as repo:
   +            repo.force_client_lock(client_name)
   +        return 0
   +
   +
        def lock(self, args):
            '''Add locks to the repository.
From: "Robin H. Johnson" <robbat2@gentoo.org>
Date: Sat, 18 Jun 2016 02:48:44 -0700

   Add commands to take & release regular locks, for external interaction.
   This permits usage such as snapshotting the repository safely.
   
   Signed-off-by: Robin H. Johnson <robbat2@gentoo.org>
   ---
    obnamlib/plugins/force_lock_plugin.py | 35 +++++++++++++++++++++++++++++++++++
    1 file changed, 35 insertions(+)
   
   diff --git a/obnamlib/plugins/force_lock_plugin.py b/obnamlib/plugins/force_lock_plugin.py
   index a14d680..ef6f131 100644
   --- a/obnamlib/plugins/force_lock_plugin.py
   +++ b/obnamlib/plugins/force_lock_plugin.py
   @@ -52,6 +52,10 @@ class ForceLockPlugin(obnamlib.ObnamPlugin):
            # new:
            self.app.add_subcommand('lock-force-release-all', self.force_release_all_locks)
            self.app.add_subcommand('lock-force-release-client', self.force_release_client_lock)
   +        self.app.add_subcommand('lock-take-all', self.take_all_locks)
   +        self.app.add_subcommand('lock-take-client', self.take_client_lock)
   +        self.app.add_subcommand('lock-release-all', self.release_all_locks)
   +        self.app.add_subcommand('lock-release-client', self.release_client_lock)
    
        def force_lock(self, args):
            '''Force a locked repository to be open.'''
   @@ -79,6 +83,37 @@ class ForceLockPlugin(obnamlib.ObnamPlugin):
                repo.force_client_lock(client_name)
            return 0
    
   +    def take_all_locks(self, args):
   +        '''Take global locks and client-specific lock'''
   +        logging.info('Taking global locks and client-specific lock')
   +        with RepoAccessWrapper(self) as repo:
   +            repo.lock_everything()
   +        return 0
   +
   +    def take_client_lock(self, args):
   +        '''Take a specific client lock.'''
   +
   +        client_name = self.app.settings['client-name']
   +        logging.info('Taking client lock for %s', client_name)
   +        with RepoAccessWrapper(self) as repo:
   +            repo.lock_client(client_name)
   +        return 0
   +
   +    def release_all_locks(self, args):
   +        '''Release global locks and client-specific lock'''
   +        logging.info('Releasing global locks and client-specific lock')
   +        with RepoAccessWrapper(self) as repo:
   +            repo.unlock_everything()
   +        return 0
   +
   +    def release_client_lock(self, args):
   +        '''Release a specific client lock.'''
   +
   +        client_name = self.app.settings['client-name']
   +        logging.info('Releasing client lock for %s', client_name)
   +        with RepoAccessWrapper(self) as repo:
   +            repo.unlock_client(client_name)
   +        return 0
    
        def lock(self, args):
            '''Add locks to the repository.
From: Lars Wirzenius <liw@liw.fi>
Date: Sun, 24 Jul 2016 13:09:08 +0300

   I'm afraid I'm rejecting this patch series as presented.
   
   A context manager repository locking is probably a good idea. Putting
   it into only one plugin is pointless.
   
   I don't agree with randomly changing subcommand names. I don't agree
   making the old name deprecated and giving warnings if it's used.
   There's no reason to make everyone go fix their existing scripts, etc,
   just because of aesthetics.
   
   I don't think it's necessary to having a strict naming scheme for the
   various locking subcommands.
   
   Also, there is no updates to documentation or tests.
   
   On Sat, Jun 18, 2016 at 02:48:44AM -0700, Robin H. Johnson wrote:
   > Add commands to take & release regular locks, for external interaction.
   > This permits usage such as snapshotting the repository safely.
   > 
   > Signed-off-by: Robin H. Johnson <robbat2@gentoo.org>
   > ---
   >  obnamlib/plugins/force_lock_plugin.py | 35 +++++++++++++++++++++++++++++++++++
   >  1 file changed, 35 insertions(+)
   > 
   > diff --git a/obnamlib/plugins/force_lock_plugin.py b/obnamlib/plugins/force_lock_plugin.py
   > index a14d680..ef6f131 100644
   > --- a/obnamlib/plugins/force_lock_plugin.py
   > +++ b/obnamlib/plugins/force_lock_plugin.py
   > @@ -52,6 +52,10 @@ class ForceLockPlugin(obnamlib.ObnamPlugin):
   >          # new:
   >          self.app.add_subcommand('lock-force-release-all', self.force_release_all_locks)
   >          self.app.add_subcommand('lock-force-release-client', self.force_release_client_lock)
   > +        self.app.add_subcommand('lock-take-all', self.take_all_locks)
   > +        self.app.add_subcommand('lock-take-client', self.take_client_lock)
   > +        self.app.add_subcommand('lock-release-all', self.release_all_locks)
   > +        self.app.add_subcommand('lock-release-client', self.release_client_lock)
   >  
   >      def force_lock(self, args):
   >          '''Force a locked repository to be open.'''
   > @@ -79,6 +83,37 @@ class ForceLockPlugin(obnamlib.ObnamPlugin):
   >              repo.force_client_lock(client_name)
   >          return 0
   >  
   > +    def take_all_locks(self, args):
   > +        '''Take global locks and client-specific lock'''
   > +        logging.info('Taking global locks and client-specific lock')
   > +        with RepoAccessWrapper(self) as repo:
   > +            repo.lock_everything()
   > +        return 0
   > +
   > +    def take_client_lock(self, args):
   > +        '''Take a specific client lock.'''
   > +
   > +        client_name = self.app.settings['client-name']
   > +        logging.info('Taking client lock for %s', client_name)
   > +        with RepoAccessWrapper(self) as repo:
   > +            repo.lock_client(client_name)
   > +        return 0
   > +
   > +    def release_all_locks(self, args):
   > +        '''Release global locks and client-specific lock'''
   > +        logging.info('Releasing global locks and client-specific lock')
   > +        with RepoAccessWrapper(self) as repo:
   > +            repo.unlock_everything()
   > +        return 0
   > +
   > +    def release_client_lock(self, args):
   > +        '''Release a specific client lock.'''
   > +
   > +        client_name = self.app.settings['client-name']
   > +        logging.info('Releasing client lock for %s', client_name)
   > +        with RepoAccessWrapper(self) as repo:
   > +            repo.unlock_client(client_name)
   > +        return 0
   >  
   >      def lock(self, args):
   >          '''Add locks to the repository.
   > -- 
   > 2.9.0
   > 
   > 
   > _______________________________________________
   > obnam-dev mailing list
   > obnam-dev@obnam.org
   > http://listmaster.pepperfish.net/cgi-bin/mailman/listinfo/obnam-dev-obnam.org
   >