On 03/08/16 16:33, Jan Cholasta wrote:
On 3.8.2016 16:23, David Kupka wrote:
On 21/07/16 10:12, Jan Cholasta wrote:
Hi,

On 20.7.2016 14:32, David Kupka wrote:
On 15/07/16 12:53, David Kupka wrote:
Hello!

After Honza introduced thin client that builds plugins and commands
dynamically from schema client became much slower. This is only
logical,
instead of importing a module client now must fetch the schema from
server, parse it and instantiate the commands using the data.

First step to speed it up was addition of schema cache to client. That
removed the RTT and download time of fetching schema every time.

Now the most time consuming task became displaying help for lists of
topics and command and displaying individual topics. This is simply
because of the need to instantiate all the commands to find the
relations between topics and commands.

All the necessary bits for server commands and topics are already in
the
schema cache so we can skip this part and generate help from it,
right?
Not so fast!

There are client plugins with commands and topics. So we can generate
basic bits (list of all topics, list of all commands, list of commands
for each topic) from schema and store it in cache. Then we need to go
through all client plugins and get similar bits for client plugins.
Then
we can merge and print.

Still the client response is not as fast as before and I this it even
can't be. Also first time you display particular topic or list takes
longer because it must be freshly generated and stored in cache for
next
use. And this is what the attached patches do.

https://fedorahosted.org/freeipa/ticket/6048

Reimplemented so there is no need to distinguish client plugins and
remote plugins.
The main idea of this approach is to avoid creating instances of the
commands just to get the information about topic, name and summary
needed for displaying help. Instead class properties are used to access
the information directly in schema.

Patch 0112:

I think this would better be done in Schema.read_namespace_member,
because Schema is where all the state is.

(BTW does _SchemaNameSpace.__getitem__ raise KeyError for non-existent
keys? It looks like it doesn't.)


Patch 0113:

How about setting _schema_cached to False in Schema.__init__() rather
that getattr()-ing it in _ensure_cached()?


Patch 0116:

ClientCommand.doc should be a class property as well, otherwise .summary
won't work on it correctly.

_SchemaCommand.doc should not be a property, as it's not needed for
.summary to work on it correctly.


Otherwise works fine for me.

Honza


Updated patches attached.

Thanks, ACK.

Pushed to master: 229e2a1ed9ea9877cb5e879fadd99f9040f77c96


I've made and reviewer overlooked some errors. Attached patches fix them.

--
David Kupka
From ae1a9d024e37a153b6e9e4ada657f0e1e78300ef Mon Sep 17 00:00:00 2001
From: David Kupka <dku...@redhat.com>
Date: Thu, 4 Aug 2016 16:02:24 +0200
Subject: [PATCH 1/3] schema cache: Do not reset ServerInfo dirty flag

Once dirty flag is set to True it must not be set back to False.
Otherwise changes are not written back to file.

https://fedorahosted.org/freeipa/ticket/6048
---
 ipaclient/remote_plugins/__init__.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/ipaclient/remote_plugins/__init__.py b/ipaclient/remote_plugins/__init__.py
index 444651d30fd0cd96299fecb7ee7b5e4532b0abd4..976d6968724088d8e1fe8d3615990accf585ffeb 100644
--- a/ipaclient/remote_plugins/__init__.py
+++ b/ipaclient/remote_plugins/__init__.py
@@ -59,7 +59,8 @@ class ServerInfo(collections.MutableMapping):
         return self._dict[key]
 
     def __setitem__(self, key, value):
-        self._dirty = key not in self._dict or self._dict[key] != value
+        if key not in self._dict or self._dict[key] != value:
+            self._dirty = True
         self._dict[key] = value
 
     def __delitem__(self, key):
-- 
2.7.4

From a6e9b17e0ad109f3237f4417828f7a3cfb5aa743 Mon Sep 17 00:00:00 2001
From: David Kupka <dku...@redhat.com>
Date: Thu, 4 Aug 2016 16:07:08 +0200
Subject: [PATCH 2/3] schema cache: Write format information in cache

When format is not in cache '0' is assumed and client expecting higher
format refuses to load such cache.

https://fedorahosted.org/freeipa/ticket/6048
---
 ipaclient/remote_plugins/schema.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/ipaclient/remote_plugins/schema.py b/ipaclient/remote_plugins/schema.py
index a215452ea0d2c1278a6121b3806b0daee02abd6e..13b1c365b111faae673c5ed78a7e5439a869c330 100644
--- a/ipaclient/remote_plugins/schema.py
+++ b/ipaclient/remote_plugins/schema.py
@@ -524,6 +524,7 @@ class Schema(object):
 
             schema.writestr('_help',
                             json.dumps(self._generate_help(self._dict)))
+            schema.writestr('format', json.dumps(FORMAT))
 
     def _read(self, path):
         with self._open_schema(self._fingerprint, 'r') as zf:
-- 
2.7.4

From c57a1b98ca518db8909d4d1176c99c4b2daec606 Mon Sep 17 00:00:00 2001
From: David Kupka <dku...@redhat.com>
Date: Thu, 4 Aug 2016 16:14:33 +0200
Subject: [PATCH 3/3] Access data for help separately

To avoid the need to read all data for a plugin from cache and actualy
use the separately stored help data it must be requested and returned
separately.

https://fedorahosted.org/freeipa/ticket/6048
---
 ipaclient/remote_plugins/schema.py | 31 ++++++++++++-------------------
 1 file changed, 12 insertions(+), 19 deletions(-)

diff --git a/ipaclient/remote_plugins/schema.py b/ipaclient/remote_plugins/schema.py
index 13b1c365b111faae673c5ed78a7e5439a869c330..4b72690ad0468d354035e934036fe014966bb50c 100644
--- a/ipaclient/remote_plugins/schema.py
+++ b/ipaclient/remote_plugins/schema.py
@@ -103,10 +103,9 @@ class _SchemaPlugin(object):
         if self._class is not None:
             return self._class.summary
         else:
-            self._schema.load_help()
-            schema = self._schema[self.schema_key][self.full_name]
+            halp = self._schema.get_help(self.schema_key, self.full_name)
             try:
-                return schema['summary']
+                return halp['summary']
             except KeyError:
                 return u'<%s>' % self.full_name
 
@@ -233,10 +232,9 @@ class _SchemaCommandPlugin(_SchemaPlugin):
         if self._class is not None:
             return self._class.topic
         else:
-            self._schema.load_help()
-            schema = self._schema[self.schema_key][self.full_name]
+            halp = self._schema.get_help(self.schema_key, self.full_name)
             try:
-                return str(schema['topic_topic']).partition('/')[0]
+                return str(halp['topic_topic']).partition('/')[0]
             except KeyError:
                 return None
 
@@ -245,9 +243,8 @@ class _SchemaCommandPlugin(_SchemaPlugin):
         if self._class is not None:
             return self._class.NO_CLI
         else:
-            self._schema.load_help()
-            schema = self._schema[self.schema_key][self.full_name]
-            return 'cli' in schema.get('exclude', [])
+            halp = self._schema.get_help(self.schema_key, self.full_name)
+            return 'cli' in halp.get('exclude', [])
 
     def _create_output(self, api, schema):
         if schema.get('multivalue', False):
@@ -475,7 +472,7 @@ class Schema(object):
             for name in schema.namelist():
                 ns, _slash, key = name.partition('/')
                 if ns in self.namespaces:
-                    self._dict[ns][key] = {}
+                    self._dict[ns][key] = None
 
     def __getitem__(self, key):
         try:
@@ -509,7 +506,7 @@ class Schema(object):
             os.makedirs(self._DIR)
         except EnvironmentError as e:
             if e.errno != errno.EEXIST:
-                logger.warning("Failed ti write schema: {}".format(e))
+                logger.warning("Failed to write schema: {}".format(e))
                 return
 
         with self._open_schema(self._fingerprint, 'w') as schema:
@@ -533,24 +530,20 @@ class Schema(object):
     def read_namespace_member(self, namespace, member):
         value = self._dict[namespace][member]
 
-        if (not value) or ('full_name' not in value):
+        if value is None:
             path = '{}/{}'.format(namespace, member)
-            value = self._dict[namespace].setdefault(
-                member, {}
-            ).update(self._read(path))
+            value = self._dict[namespace][member] = self._read(path)
 
         return value
 
     def iter_namespace(self, namespace):
         return iter(self._dict[namespace])
 
-    def load_help(self):
+    def get_help(self, namespace, member):
         if not self._help:
             self._help = self._read('_help')
 
-            for ns in self._help:
-                for member in self._help[ns]:
-                    self._dict[ns][member].update(self._help[ns][member])
+        return self._help[namespace][member]
 
 
 def get_package(api, server_info, client):
-- 
2.7.4

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to