Обсуждение: [pgadmin-hackers] Feature test regression failures

Поиск
Список
Период
Сортировка

[pgadmin-hackers] Feature test regression failures

От
Dave Page
Дата:
Hi Ashesh,

A common theme is emerging from some of the feature test regression
failures on the Jenkins server. Please see:


https://jenkins.pgadmin.org/job/pgadmin4-master-python27/ws/web/regression/screenshots/EDB_Postgres_AS_9.3/ConnectsToServerFeatureTest-2017.03.16_10.09.18-Python-2.7.13.png

I've very occasionally seen similar behaviour to this in the past - in
fact it's part of the reason why we grey out the UI until pgAdmin is
fully loaded.

Any idea what might be causing it?

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


Re: [pgadmin-hackers] Feature test regression failures

От
Ashesh Vashi
Дата:

On Thu, Mar 16, 2017 at 3:55 PM, Dave Page <dpage@pgadmin.org> wrote:

Hi Ashesh,

A common theme is emerging from some of the feature test regression
failures on the Jenkins server. Please see:

https://jenkins.pgadmin.org/job/pgadmin4-master-python27/ws/web/regression/screenshots/EDB_Postgres_AS_9.3/ConnectsToServerFeatureTest-2017.03.16_10.09.18-Python-2.7.13.png

I've very occasionally seen similar behaviour to this in the past - in
fact it's part of the reason why we grey out the UI until pgAdmin is
fully loaded.

Any idea what might be causing it?
This can happen, when the module is not yet loaded for the respective node, and it is being expanded.
Just thinking - shall we load all the javascript in the beginning?

--

Thanks & Regards,

Ashesh Vashi
EnterpriseDB INDIA: Enterprise PostgreSQL Company


http://www.linkedin.com/in/asheshvashi


--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [pgadmin-hackers] Feature test regression failures

От
Dave Page
Дата:


On Thu, Mar 16, 2017 at 10:39 AM, Ashesh Vashi <ashesh.vashi@enterprisedb.com> wrote:

On Thu, Mar 16, 2017 at 3:55 PM, Dave Page <dpage@pgadmin.org> wrote:

Hi Ashesh,

A common theme is emerging from some of the feature test regression
failures on the Jenkins server. Please see:

https://jenkins.pgadmin.org/job/pgadmin4-master-python27/ws/web/regression/screenshots/EDB_Postgres_AS_9.3/ConnectsToServerFeatureTest-2017.03.16_10.09.18-Python-2.7.13.png

I've very occasionally seen similar behaviour to this in the past - in
fact it's part of the reason why we grey out the UI until pgAdmin is
fully loaded.

Any idea what might be causing it?
This can happen, when the module is not yet loaded for the respective node, and it is being expanded.
Just thinking - shall we load all the javascript in the beginning?

That could be a lot of code, especially as the app grows. It may not be an issue in the runtime (though, some recent reports would imply otherwise), but it almost certainly would be on slower connections to installations running in server mode. 

There must be a way to ensure the code is loaded before we allow it to be used?

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [pgadmin-hackers] Feature test regression failures

От
Atira Odhner
Дата:

I have seen this issue as well.

Ashesh, this issue is related to the loading of the tree node data, not loading of code, correct? Each time the user expands a node triggers an ajax request to fetch the child nodes. There are probably some performance tradeoffs to loading that tree up front.

But, there are ways to solve this issue without doing that. We could use callbacks/promises to wait for things to be loaded before rendering the node in an enabled/expandable state. It might be helpful to use a one-way data flow redux pattern to manage the state and rendering of the tree nodes.

Tira

On Thu, Mar 16, 2017, 6:49 AM Dave Page <dpage@pgadmin.org> wrote:
On Thu, Mar 16, 2017 at 10:39 AM, Ashesh Vashi <ashesh.vashi@enterprisedb.com> wrote:

On Thu, Mar 16, 2017 at 3:55 PM, Dave Page <dpage@pgadmin.org> wrote:

Hi Ashesh,

A common theme is emerging from some of the feature test regression
failures on the Jenkins server. Please see:

https://jenkins.pgadmin.org/job/pgadmin4-master-python27/ws/web/regression/screenshots/EDB_Postgres_AS_9.3/ConnectsToServerFeatureTest-2017.03.16_10.09.18-Python-2.7.13.png

I've very occasionally seen similar behaviour to this in the past - in
fact it's part of the reason why we grey out the UI until pgAdmin is
fully loaded.

Any idea what might be causing it?
This can happen, when the module is not yet loaded for the respective node, and it is being expanded.
Just thinking - shall we load all the javascript in the beginning?

That could be a lot of code, especially as the app grows. It may not be an issue in the runtime (though, some recent reports would imply otherwise), but it almost certainly would be on slower connections to installations running in server mode. 

There must be a way to ensure the code is loaded before we allow it to be used?


--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [pgadmin-hackers] Feature test regression failures

От
Ashesh Vashi
Дата:


On Mar 16, 2017 22:32, "Atira Odhner" <aodhner@pivotal.io> wrote:

I have seen this issue as well.

Ashesh, this issue is related to the loading of the tree node data, not loading of code, correct?

Theoritically - Each node may contain code to represent the node url. For all current nodes follow the function (present in all inherited class for nodes) to generated URI. 

So - indirectly it relies on the individual node code.

Each time the user expands a node triggers an ajax request to fetch the child nodes. There are probably some performance tradeoffs to loading that tree up front.

Agree.
That was the reason, we load them only when first parent node (in some cases grand parent node) is loaded.

But, there are ways to solve this issue without doing that. We could use callbacks/promises to wait for things to be loaded before rendering the node in an enabled/expandable state. It might be helpful to use a one-way data flow redux pattern to manage the state and rendering of the tree nodes.

We're relying on the third party library jquery.acitree for tree operations.
Most operations can be done asynchronous.  But - url generation for individual node is done using callbacks, which can not work in asynchronous mode.

I am thinking of using the 'require' function within that function, when that node type is not present in the application at that point of time. Though - I am still not sure, wheather 'require' works that way, or not.

-- Thanks, Ashesh

Tira


On Thu, Mar 16, 2017, 6:49 AM Dave Page <dpage@pgadmin.org> wrote:
On Thu, Mar 16, 2017 at 10:39 AM, Ashesh Vashi <ashesh.vashi@enterprisedb.com> wrote:

On Thu, Mar 16, 2017 at 3:55 PM, Dave Page <dpage@pgadmin.org> wrote:

Hi Ashesh,

A common theme is emerging from some of the feature test regression
failures on the Jenkins server. Please see:

https://jenkins.pgadmin.org/job/pgadmin4-master-python27/ws/web/regression/screenshots/EDB_Postgres_AS_9.3/ConnectsToServerFeatureTest-2017.03.16_10.09.18-Python-2.7.13.png

I've very occasionally seen similar behaviour to this in the past - in
fact it's part of the reason why we grey out the UI until pgAdmin is
fully loaded.

Any idea what might be causing it?
This can happen, when the module is not yet loaded for the respective node, and it is being expanded.
Just thinking - shall we load all the javascript in the beginning?

That could be a lot of code, especially as the app grows. It may not be an issue in the runtime (though, some recent reports would imply otherwise), but it almost certainly would be on slower connections to installations running in server mode. 

There must be a way to ensure the code is loaded before we allow it to be used?


--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [pgadmin-hackers] Feature test regression failures

От
Atira Odhner
Дата:
We're relying on the third party library jquery.acitree for tree operations.

Yes, what I was suggesting is to probably move away from that. AciTree is one of the libraries my team identified as questionable (not in major repositories, not actively maintained) and there are lots of alternatives.

That said, I took a peek at the code and noticed that it was using a global variable ('d') inside the loop where it calculated the tree hierarchy. I suspect that is not the only issue, but here is a patch for it.

Tira

On Thu, Mar 16, 2017 at 1:39 PM, Ashesh Vashi <ashesh.vashi@enterprisedb.com> wrote:


On Mar 16, 2017 22:32, "Atira Odhner" <aodhner@pivotal.io> wrote:

I have seen this issue as well.

Ashesh, this issue is related to the loading of the tree node data, not loading of code, correct?

Theoritically - Each node may contain code to represent the node url. For all current nodes follow the function (present in all inherited class for nodes) to generated URI. 

So - indirectly it relies on the individual node code.

Each time the user expands a node triggers an ajax request to fetch the child nodes. There are probably some performance tradeoffs to loading that tree up front.

Agree.
That was the reason, we load them only when first parent node (in some cases grand parent node) is loaded.

But, there are ways to solve this issue without doing that. We could use callbacks/promises to wait for things to be loaded before rendering the node in an enabled/expandable state. It might be helpful to use a one-way data flow redux pattern to manage the state and rendering of the tree nodes.

We're relying on the third party library jquery.acitree for tree operations.
Most operations can be done asynchronous.  But - url generation for individual node is done using callbacks, which can not work in asynchronous mode.

I am thinking of using the 'require' function within that function, when that node type is not present in the application at that point of time. Though - I am still not sure, wheather 'require' works that way, or not.

-- Thanks, Ashesh

Tira


On Thu, Mar 16, 2017, 6:49 AM Dave Page <dpage@pgadmin.org> wrote:
On Thu, Mar 16, 2017 at 10:39 AM, Ashesh Vashi <ashesh.vashi@enterprisedb.com> wrote:

On Thu, Mar 16, 2017 at 3:55 PM, Dave Page <dpage@pgadmin.org> wrote:

Hi Ashesh,

A common theme is emerging from some of the feature test regression
failures on the Jenkins server. Please see:

https://jenkins.pgadmin.org/job/pgadmin4-master-python27/ws/web/regression/screenshots/EDB_Postgres_AS_9.3/ConnectsToServerFeatureTest-2017.03.16_10.09.18-Python-2.7.13.png

I've very occasionally seen similar behaviour to this in the past - in
fact it's part of the reason why we grey out the UI until pgAdmin is
fully loaded.

Any idea what might be causing it?
This can happen, when the module is not yet loaded for the respective node, and it is being expanded.
Just thinking - shall we load all the javascript in the beginning?

That could be a lot of code, especially as the app grows. It may not be an issue in the runtime (though, some recent reports would imply otherwise), but it almost certainly would be on slower connections to installations running in server mode. 

There must be a way to ensure the code is loaded before we allow it to be used?


--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


Вложения

Re: [pgadmin-hackers] Feature test regression failures

От
Sarah McAlear
Дата:
Hello,

We agree that we should keep an eye on this and the failing feature tests. Our current story touches part of this code, but we won't go into changing the library for now. 

The patch Tira sent fixes a global variable problem that we found while looking into the code that generates the Tree, that 
had the potential to load the tree in an infinite loop.
Can you apply the patch like this, or would you rather us send it in a separate patch email?

Thanks!
Joao and Sarah

On Thu, Mar 16, 2017 at 10:56 PM, Atira Odhner <aodhner@pivotal.io> wrote:
We're relying on the third party library jquery.acitree for tree operations.

Yes, what I was suggesting is to probably move away from that. AciTree is one of the libraries my team identified as questionable (not in major repositories, not actively maintained) and there are lots of alternatives.

That said, I took a peek at the code and noticed that it was using a global variable ('d') inside the loop where it calculated the tree hierarchy. I suspect that is not the only issue, but here is a patch for it.

Tira

On Thu, Mar 16, 2017 at 1:39 PM, Ashesh Vashi <ashesh.vashi@enterprisedb.com> wrote:


On Mar 16, 2017 22:32, "Atira Odhner" <aodhner@pivotal.io> wrote:

I have seen this issue as well.

Ashesh, this issue is related to the loading of the tree node data, not loading of code, correct?

Theoritically - Each node may contain code to represent the node url. For all current nodes follow the function (present in all inherited class for nodes) to generated URI. 

So - indirectly it relies on the individual node code.

Each time the user expands a node triggers an ajax request to fetch the child nodes. There are probably some performance tradeoffs to loading that tree up front.

Agree.
That was the reason, we load them only when first parent node (in some cases grand parent node) is loaded.

But, there are ways to solve this issue without doing that. We could use callbacks/promises to wait for things to be loaded before rendering the node in an enabled/expandable state. It might be helpful to use a one-way data flow redux pattern to manage the state and rendering of the tree nodes.

We're relying on the third party library jquery.acitree for tree operations.
Most operations can be done asynchronous.  But - url generation for individual node is done using callbacks, which can not work in asynchronous mode.

I am thinking of using the 'require' function within that function, when that node type is not present in the application at that point of time. Though - I am still not sure, wheather 'require' works that way, or not.

-- Thanks, Ashesh

Tira


On Thu, Mar 16, 2017, 6:49 AM Dave Page <dpage@pgadmin.org> wrote:
On Thu, Mar 16, 2017 at 10:39 AM, Ashesh Vashi <ashesh.vashi@enterprisedb.com> wrote:

On Thu, Mar 16, 2017 at 3:55 PM, Dave Page <dpage@pgadmin.org> wrote:

Hi Ashesh,

A common theme is emerging from some of the feature test regression
failures on the Jenkins server. Please see:

https://jenkins.pgadmin.org/job/pgadmin4-master-python27/ws/web/regression/screenshots/EDB_Postgres_AS_9.3/ConnectsToServerFeatureTest-2017.03.16_10.09.18-Python-2.7.13.png

I've very occasionally seen similar behaviour to this in the past - in
fact it's part of the reason why we grey out the UI until pgAdmin is
fully loaded.

Any idea what might be causing it?
This can happen, when the module is not yet loaded for the respective node, and it is being expanded.
Just thinking - shall we load all the javascript in the beginning?

That could be a lot of code, especially as the app grows. It may not be an issue in the runtime (though, some recent reports would imply otherwise), but it almost certainly would be on slower connections to installations running in server mode. 

There must be a way to ensure the code is loaded before we allow it to be used?


--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company




--
Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers


Re: [pgadmin-hackers] Feature test regression failures

От
Ashesh Vashi
Дата:
On Fri, Mar 17, 2017 at 8:35 PM, Sarah McAlear <smcalear@pivotal.io> wrote:
Hello,

We agree that we should keep an eye on this and the failing feature tests. Our current story touches part of this code, but we won't go into changing the library for now. 

The patch Tira sent fixes a global variable problem that we found while looking into the code that generates the Tree, that 
had the potential to load the tree in an infinite loop.
Can you apply the patch like this, or would you rather us send it in a separate patch email?
Name of the variable should have been itemData, d (as earlier), as it represents the data for the expanding node item.
And, that is not good enough for resolving the issue.

I've two approaches to resolve the idea.
1. Load the nodes (even when the module representing that node is not yet loaded)
Pros:
- Nodes will be loaded even when the module for the node type is not yet loaded.
Cons:
- The nodes with modified url (not the default mechanism) may get failed, if the module is not yet loaded.
  (NOTE: We've not no nodes with that changes at the moment. so - we can safely ignore it.)
- Operations specific to the nodes will not be honoured until modules is not loaded.

2. Wait for the modules to get loaded before allowing any operations on them.
Pros:
- All operations will be done on a node only after the respective module is loaded.
Cons:
- It will block any operations on pgAdmin 4, when the dependent modules are being loaded. It can annoy the user.

Please find the patches for both the approaches.

Dave - please take a look at it.

-- Thanks, Ashesh


Thanks!
Joao and Sarah

On Thu, Mar 16, 2017 at 10:56 PM, Atira Odhner <aodhner@pivotal.io> wrote:
We're relying on the third party library jquery.acitree for tree operations.

Yes, what I was suggesting is to probably move away from that. AciTree is one of the libraries my team identified as questionable (not in major repositories, not actively maintained) and there are lots of alternatives.

That said, I took a peek at the code and noticed that it was using a global variable ('d') inside the loop where it calculated the tree hierarchy. I suspect that is not the only issue, but here is a patch for it.

Tira

On Thu, Mar 16, 2017 at 1:39 PM, Ashesh Vashi <ashesh.vashi@enterprisedb.com> wrote:


On Mar 16, 2017 22:32, "Atira Odhner" <aodhner@pivotal.io> wrote:

I have seen this issue as well.

Ashesh, this issue is related to the loading of the tree node data, not loading of code, correct?

Theoritically - Each node may contain code to represent the node url. For all current nodes follow the function (present in all inherited class for nodes) to generated URI. 

So - indirectly it relies on the individual node code.

Each time the user expands a node triggers an ajax request to fetch the child nodes. There are probably some performance tradeoffs to loading that tree up front.

Agree.
That was the reason, we load them only when first parent node (in some cases grand parent node) is loaded.

But, there are ways to solve this issue without doing that. We could use callbacks/promises to wait for things to be loaded before rendering the node in an enabled/expandable state. It might be helpful to use a one-way data flow redux pattern to manage the state and rendering of the tree nodes.

We're relying on the third party library jquery.acitree for tree operations.
Most operations can be done asynchronous.  But - url generation for individual node is done using callbacks, which can not work in asynchronous mode.

I am thinking of using the 'require' function within that function, when that node type is not present in the application at that point of time. Though - I am still not sure, wheather 'require' works that way, or not.

-- Thanks, Ashesh

Tira


On Thu, Mar 16, 2017, 6:49 AM Dave Page <dpage@pgadmin.org> wrote:
On Thu, Mar 16, 2017 at 10:39 AM, Ashesh Vashi <ashesh.vashi@enterprisedb.com> wrote:

On Thu, Mar 16, 2017 at 3:55 PM, Dave Page <dpage@pgadmin.org> wrote:

Hi Ashesh,

A common theme is emerging from some of the feature test regression
failures on the Jenkins server. Please see:

https://jenkins.pgadmin.org/job/pgadmin4-master-python27/ws/web/regression/screenshots/EDB_Postgres_AS_9.3/ConnectsToServerFeatureTest-2017.03.16_10.09.18-Python-2.7.13.png

I've very occasionally seen similar behaviour to this in the past - in
fact it's part of the reason why we grey out the UI until pgAdmin is
fully loaded.

Any idea what might be causing it?
This can happen, when the module is not yet loaded for the respective node, and it is being expanded.
Just thinking - shall we load all the javascript in the beginning?

That could be a lot of code, especially as the app grows. It may not be an issue in the runtime (though, some recent reports would imply otherwise), but it almost certainly would be on slower connections to installations running in server mode. 

There must be a way to ensure the code is loaded before we allow it to be used?


--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company




--
Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers



Вложения

Re: [pgadmin-hackers] Feature test regression failures

От
Dave Page
Дата:
On Mon, Mar 20, 2017 at 10:24 AM, Ashesh Vashi
<ashesh.vashi@enterprisedb.com> wrote:
> On Fri, Mar 17, 2017 at 8:35 PM, Sarah McAlear <smcalear@pivotal.io> wrote:
>>
>> Hello,
>>
>> We agree that we should keep an eye on this and the failing feature tests.
>> Our current story touches part of this code, but we won't go into changing
>> the library for now.
>>
>> The patch Tira sent fixes a global variable problem that we found while
>> looking into the code that generates the Tree, that
>> had the potential to load the tree in an infinite loop.
>> Can you apply the patch like this, or would you rather us send it in a
>> separate patch email?
>
> Name of the variable should have been itemData, d (as earlier), as it
> represents the data for the expanding node item.
> And, that is not good enough for resolving the issue.
>
> I've two approaches to resolve the idea.
> 1. Load the nodes (even when the module representing that node is not yet
> loaded)
> Pros:
> - Nodes will be loaded even when the module for the node type is not yet
> loaded.
> Cons:
> - The nodes with modified url (not the default mechanism) may get failed, if
> the module is not yet loaded.
>   (NOTE: We've not no nodes with that changes at the moment. so - we can
> safely ignore it.)
> - Operations specific to the nodes will not be honoured until modules is not
> loaded.
>
> 2. Wait for the modules to get loaded before allowing any operations on
> them.
> Pros:
> - All operations will be done on a node only after the respective module is
> loaded.
> Cons:
> - It will block any operations on pgAdmin 4, when the dependent modules are
> being loaded. It can annoy the user.
>
> Please find the patches for both the approaches.
>
> Dave - please take a look at it.

I'll leave you and Tira to figure this one out if you don't mind. My
plate is kinda full at the moment.

I will note though that neither blocking or potential failures are desirable.

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


Re: [pgadmin-hackers] Feature test regression failures

От
Atira Odhner
Дата:

Hi Ashesh,

Regarding your second patch:

It looks like your second patch addresses module loading. This is an improvement over the previous hard timeout, but won’t do anything for the tree issues. The module loading code can also be simplified; we’ve attached a patchset that is tidier, tests the behavior, and speeds up the polling.

Ashesh, can you explain why you are setting the text on the spinner after hiding it, or why you are hiding it rather than removing it?

Regarding your first patch:

Descriptive variable names are clearer than single-letter variable names. Could you clean up the first patch to use clearer variables, perhaps add some tests and break it up into methods so that I can more easily understand what your change does?

Thank you,

Tira & George


On Mon, Mar 20, 2017 at 8:03 AM, Dave Page <dpage@pgadmin.org> wrote:
On Mon, Mar 20, 2017 at 10:24 AM, Ashesh Vashi
<ashesh.vashi@enterprisedb.com> wrote:
> On Fri, Mar 17, 2017 at 8:35 PM, Sarah McAlear <smcalear@pivotal.io> wrote:
>>
>> Hello,
>>
>> We agree that we should keep an eye on this and the failing feature tests.
>> Our current story touches part of this code, but we won't go into changing
>> the library for now.
>>
>> The patch Tira sent fixes a global variable problem that we found while
>> looking into the code that generates the Tree, that
>> had the potential to load the tree in an infinite loop.
>> Can you apply the patch like this, or would you rather us send it in a
>> separate patch email?
>
> Name of the variable should have been itemData, d (as earlier), as it
> represents the data for the expanding node item.
> And, that is not good enough for resolving the issue.
>
> I've two approaches to resolve the idea.
> 1. Load the nodes (even when the module representing that node is not yet
> loaded)
> Pros:
> - Nodes will be loaded even when the module for the node type is not yet
> loaded.
> Cons:
> - The nodes with modified url (not the default mechanism) may get failed, if
> the module is not yet loaded.
>   (NOTE: We've not no nodes with that changes at the moment. so - we can
> safely ignore it.)
> - Operations specific to the nodes will not be honoured until modules is not
> loaded.
>
> 2. Wait for the modules to get loaded before allowing any operations on
> them.
> Pros:
> - All operations will be done on a node only after the respective module is
> loaded.
> Cons:
> - It will block any operations on pgAdmin 4, when the dependent modules are
> being loaded. It can annoy the user.
>
> Please find the patches for both the approaches.
>
> Dave - please take a look at it.

I'll leave you and Tira to figure this one out if you don't mind. My
plate is kinda full at the moment.

I will note though that neither blocking or potential failures are desirable.

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Вложения

Re: [pgadmin-hackers] Feature test regression failures

От
Atira Odhner
Дата:
Note that this patch makes the problem of the tree not having loaded worse, because it only waits for js modules to load rather than arbitrarily waiting 900ms.

On Mon, Mar 20, 2017 at 3:17 PM, Atira Odhner <aodhner@pivotal.io> wrote:

Hi Ashesh,

Regarding your second patch:

It looks like your second patch addresses module loading. This is an improvement over the previous hard timeout, but won’t do anything for the tree issues. The module loading code can also be simplified; we’ve attached a patchset that is tidier, tests the behavior, and speeds up the polling.

Ashesh, can you explain why you are setting the text on the spinner after hiding it, or why you are hiding it rather than removing it?

Regarding your first patch:

Descriptive variable names are clearer than single-letter variable names. Could you clean up the first patch to use clearer variables, perhaps add some tests and break it up into methods so that I can more easily understand what your change does?

Thank you,

Tira & George


On Mon, Mar 20, 2017 at 8:03 AM, Dave Page <dpage@pgadmin.org> wrote:
On Mon, Mar 20, 2017 at 10:24 AM, Ashesh Vashi
<ashesh.vashi@enterprisedb.com> wrote:
> On Fri, Mar 17, 2017 at 8:35 PM, Sarah McAlear <smcalear@pivotal.io> wrote:
>>
>> Hello,
>>
>> We agree that we should keep an eye on this and the failing feature tests.
>> Our current story touches part of this code, but we won't go into changing
>> the library for now.
>>
>> The patch Tira sent fixes a global variable problem that we found while
>> looking into the code that generates the Tree, that
>> had the potential to load the tree in an infinite loop.
>> Can you apply the patch like this, or would you rather us send it in a
>> separate patch email?
>
> Name of the variable should have been itemData, d (as earlier), as it
> represents the data for the expanding node item.
> And, that is not good enough for resolving the issue.
>
> I've two approaches to resolve the idea.
> 1. Load the nodes (even when the module representing that node is not yet
> loaded)
> Pros:
> - Nodes will be loaded even when the module for the node type is not yet
> loaded.
> Cons:
> - The nodes with modified url (not the default mechanism) may get failed, if
> the module is not yet loaded.
>   (NOTE: We've not no nodes with that changes at the moment. so - we can
> safely ignore it.)
> - Operations specific to the nodes will not be honoured until modules is not
> loaded.
>
> 2. Wait for the modules to get loaded before allowing any operations on
> them.
> Pros:
> - All operations will be done on a node only after the respective module is
> loaded.
> Cons:
> - It will block any operations on pgAdmin 4, when the dependent modules are
> being loaded. It can annoy the user.
>
> Please find the patches for both the approaches.
>
> Dave - please take a look at it.

I'll leave you and Tira to figure this one out if you don't mind. My
plate is kinda full at the moment.

I will note though that neither blocking or potential failures are desirable.

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


Re: [pgadmin-hackers] Feature test regression failures

От
Atira Odhner
Дата:
Here is a new patchset that instead hides the spinner when the acitree has been initialized. On average, the spinner seems to disappear about 2 seconds sooner, and I haven't seen flakiness with these changes yet.

Tira & Joao

On Mon, Mar 20, 2017 at 4:17 PM, Atira Odhner <aodhner@pivotal.io> wrote:
Note that this patch makes the problem of the tree not having loaded worse, because it only waits for js modules to load rather than arbitrarily waiting 900ms.

On Mon, Mar 20, 2017 at 3:17 PM, Atira Odhner <aodhner@pivotal.io> wrote:

Hi Ashesh,

Regarding your second patch:

It looks like your second patch addresses module loading. This is an improvement over the previous hard timeout, but won’t do anything for the tree issues. The module loading code can also be simplified; we’ve attached a patchset that is tidier, tests the behavior, and speeds up the polling.

Ashesh, can you explain why you are setting the text on the spinner after hiding it, or why you are hiding it rather than removing it?

Regarding your first patch:

Descriptive variable names are clearer than single-letter variable names. Could you clean up the first patch to use clearer variables, perhaps add some tests and break it up into methods so that I can more easily understand what your change does?

Thank you,

Tira & George


On Mon, Mar 20, 2017 at 8:03 AM, Dave Page <dpage@pgadmin.org> wrote:
On Mon, Mar 20, 2017 at 10:24 AM, Ashesh Vashi
<ashesh.vashi@enterprisedb.com> wrote:
> On Fri, Mar 17, 2017 at 8:35 PM, Sarah McAlear <smcalear@pivotal.io> wrote:
>>
>> Hello,
>>
>> We agree that we should keep an eye on this and the failing feature tests.
>> Our current story touches part of this code, but we won't go into changing
>> the library for now.
>>
>> The patch Tira sent fixes a global variable problem that we found while
>> looking into the code that generates the Tree, that
>> had the potential to load the tree in an infinite loop.
>> Can you apply the patch like this, or would you rather us send it in a
>> separate patch email?
>
> Name of the variable should have been itemData, d (as earlier), as it
> represents the data for the expanding node item.
> And, that is not good enough for resolving the issue.
>
> I've two approaches to resolve the idea.
> 1. Load the nodes (even when the module representing that node is not yet
> loaded)
> Pros:
> - Nodes will be loaded even when the module for the node type is not yet
> loaded.
> Cons:
> - The nodes with modified url (not the default mechanism) may get failed, if
> the module is not yet loaded.
>   (NOTE: We've not no nodes with that changes at the moment. so - we can
> safely ignore it.)
> - Operations specific to the nodes will not be honoured until modules is not
> loaded.
>
> 2. Wait for the modules to get loaded before allowing any operations on
> them.
> Pros:
> - All operations will be done on a node only after the respective module is
> loaded.
> Cons:
> - It will block any operations on pgAdmin 4, when the dependent modules are
> being loaded. It can annoy the user.
>
> Please find the patches for both the approaches.
>
> Dave - please take a look at it.

I'll leave you and Tira to figure this one out if you don't mind. My
plate is kinda full at the moment.

I will note though that neither blocking or potential failures are desirable.

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company



Вложения

Re: [pgadmin-hackers] Feature test regression failures

От
Ashesh Vashi
Дата:


--

Thanks & Regards,

Ashesh Vashi
EnterpriseDB INDIA: Enterprise PostgreSQL Company


http://www.linkedin.com/in/asheshvashi


On Tue, Mar 21, 2017 at 9:20 PM, Atira Odhner <aodhner@pivotal.io> wrote:
Here is a new patchset that instead hides the spinner when the acitree has been initialized. On average, the spinner seems to disappear about 2 seconds sooner, and I haven't seen flakiness with these changes yet.

Tira & Joao

On Mon, Mar 20, 2017 at 4:17 PM, Atira Odhner <aodhner@pivotal.io> wrote:
Note that this patch makes the problem of the tree not having loaded worse, because it only waits for js modules to load rather than arbitrarily waiting 900ms.
The patch was never intended to remove the spinner earlier.
Idea was: the spinner should be hidden only after all dependent javascript modules are loaded.

I agree about using arbitrary wait was never a good option for sure.
Though - I still see the flaw in my approach.
If the dependent script has error, it wont get loaded, and can make the things worse by not hiding the spinner at all.

I liked the idea behind your first patch about using the tree events to hide the spinner.
Though - I see scope of improvement in it.

On Mon, Mar 20, 2017 at 3:17 PM, Atira Odhner <aodhner@pivotal.io> wrote:

Hi Ashesh,

Regarding your second patch:

It looks like your second patch addresses module loading. This is an improvement over the previous hard timeout, but won’t do anything for the tree issues. The module loading code can also be simplified; we’ve attached a patchset that is tidier, tests the behavior, and speeds up the polling.

Ashesh, can you explain why you are setting the text on the spinner after hiding it, or why you are hiding it rather than removing it?

First - let me try to explain the problem with the failure in the feature test.
We do not load all the javascript libraries, when starting the pgAdmin 4 (i.e. the loading the browser/index.html).
But - load them only when first tree-item of certain type is added.
e.g. We load the javascript module of 'table', 'index', etc only after first tree item of database is added in the tree.

Now - when we run the feature tests, it expands all the nodes one by one very quickly.
And, that makes the thing worse, as the javascript module for 'table' is still not loaded in the browser, definitely - not immediately after the first 'database' item is added, it always takes some time to load the module, and takes few fraction of seconds/milliseconds.

Now - the intention was show the spinner, when the dependent javascript modules gets loaded.
Hence - we did not remove the spinner, but - instead only hide it.

And, changed the text to 'Loading the dependent resources...', so that - whenever we once again show the spinner, it will show that text.
I am considering using the tree events to show the spinner again using the similar approach used in your first patch.

Then - change the feature test to wait for the spinner to go away before expanding the table node.

Regarding your first patch:

Descriptive variable names are clearer than single-letter variable names. Could you clean up the first patch to use clearer variables, perhaps add some tests and break it up into methods so that I can more easily understand what your change does?

Agree.
But - as I said earlier, it represents the data for the tree-item (not the item itself).
Hence -  it should be 'itemData', and not 'item'.

We've already used 'd' to represent the data, 'i' for item, 't' for tree at all other places in pgAdmin 4.
So - it is consistent across the application.

I would add comments to understand the functionality a lot better.

Will share the patch for the same.


-- Thanks, Ashesh

Thank you,

Tira & George


On Mon, Mar 20, 2017 at 8:03 AM, Dave Page <dpage@pgadmin.org> wrote:
On Mon, Mar 20, 2017 at 10:24 AM, Ashesh Vashi
<ashesh.vashi@enterprisedb.com> wrote:
> On Fri, Mar 17, 2017 at 8:35 PM, Sarah McAlear <smcalear@pivotal.io> wrote:
>>
>> Hello,
>>
>> We agree that we should keep an eye on this and the failing feature tests.
>> Our current story touches part of this code, but we won't go into changing
>> the library for now.
>>
>> The patch Tira sent fixes a global variable problem that we found while
>> looking into the code that generates the Tree, that
>> had the potential to load the tree in an infinite loop.
>> Can you apply the patch like this, or would you rather us send it in a
>> separate patch email?
>
> Name of the variable should have been itemData, d (as earlier), as it
> represents the data for the expanding node item.
> And, that is not good enough for resolving the issue.
>
> I've two approaches to resolve the idea.
> 1. Load the nodes (even when the module representing that node is not yet
> loaded)
> Pros:
> - Nodes will be loaded even when the module for the node type is not yet
> loaded.
> Cons:
> - The nodes with modified url (not the default mechanism) may get failed, if
> the module is not yet loaded.
>   (NOTE: We've not no nodes with that changes at the moment. so - we can
> safely ignore it.)
> - Operations specific to the nodes will not be honoured until modules is not
> loaded.
>
> 2. Wait for the modules to get loaded before allowing any operations on
> them.
> Pros:
> - All operations will be done on a node only after the respective module is
> loaded.
> Cons:
> - It will block any operations on pgAdmin 4, when the dependent modules are
> being loaded. It can annoy the user.
>
> Please find the patches for both the approaches.
>
> Dave - please take a look at it.

I'll leave you and Tira to figure this one out if you don't mind. My
plate is kinda full at the moment.

I will note though that neither blocking or potential failures are desirable.

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company




Re: [pgadmin-hackers] Feature test regression failures

От
Atira Odhner
Дата:
 First - let me try to explain the problem with the failure in the feature test.
We do not load all the javascript libraries, when starting the pgAdmin 4 (i.e. the loading the browser/index.html).
But - load them only when first tree-item of certain type is added.
e.g. We load the javascript module of 'table', 'index', etc only after first tree item of database is added in the tree.

I've seen this bug opening the tree by hand, so it is not merely a matter of opening tree items as quickly as a machine. There are two different code paths for loading the tree state --- the way the initial load happens upon expand and the call that is made when the user clicks 'Refresh...' in the context menu. Maybe a good approach to starting to fix this bug would be to have only one code path for loading tree state. I don't think this has to do with loading the javascript modules.
 
But - as I said earlier, it represents the data for the tree-item (not the item itself).
Hence -  it should be 'itemData', and not 'item'.

'itemData' and 'item' are both fine with me.
 
We've already used 'd' to represent the data, 'i' for item, 't' for tree at all other places in pgAdmin 4.
So - it is consistent across the application.

Consistency is great, but in this case, aiming for consistency is hindering the legibility of the application code. At some point, we should go through the whole application and change all the 'i' to 'item', 'd' to data', etc. Until then, I think it is worthwhile to sacrifice consistency in exchange for adding meaning and clarity to the code that is being updated.

Tira & Joao

On Wed, Mar 22, 2017 at 2:20 AM, Ashesh Vashi <ashesh.vashi@enterprisedb.com> wrote:


--

Thanks & Regards,

Ashesh Vashi
EnterpriseDB INDIA: Enterprise PostgreSQL Company


http://www.linkedin.com/in/asheshvashi


On Tue, Mar 21, 2017 at 9:20 PM, Atira Odhner <aodhner@pivotal.io> wrote:
Here is a new patchset that instead hides the spinner when the acitree has been initialized. On average, the spinner seems to disappear about 2 seconds sooner, and I haven't seen flakiness with these changes yet.

Tira & Joao

On Mon, Mar 20, 2017 at 4:17 PM, Atira Odhner <aodhner@pivotal.io> wrote:
Note that this patch makes the problem of the tree not having loaded worse, because it only waits for js modules to load rather than arbitrarily waiting 900ms.
The patch was never intended to remove the spinner earlier.
Idea was: the spinner should be hidden only after all dependent javascript modules are loaded.

I agree about using arbitrary wait was never a good option for sure.
Though - I still see the flaw in my approach.
If the dependent script has error, it wont get loaded, and can make the things worse by not hiding the spinner at all.

I liked the idea behind your first patch about using the tree events to hide the spinner.
Though - I see scope of improvement in it.

On Mon, Mar 20, 2017 at 3:17 PM, Atira Odhner <aodhner@pivotal.io> wrote:

Hi Ashesh,

Regarding your second patch:

It looks like your second patch addresses module loading. This is an improvement over the previous hard timeout, but won’t do anything for the tree issues. The module loading code can also be simplified; we’ve attached a patchset that is tidier, tests the behavior, and speeds up the polling.

Ashesh, can you explain why you are setting the text on the spinner after hiding it, or why you are hiding it rather than removing it?

First - let me try to explain the problem with the failure in the feature test.
We do not load all the javascript libraries, when starting the pgAdmin 4 (i.e. the loading the browser/index.html).
But - load them only when first tree-item of certain type is added.
e.g. We load the javascript module of 'table', 'index', etc only after first tree item of database is added in the tree.

Now - when we run the feature tests, it expands all the nodes one by one very quickly.
And, that makes the thing worse, as the javascript module for 'table' is still not loaded in the browser, definitely - not immediately after the first 'database' item is added, it always takes some time to load the module, and takes few fraction of seconds/milliseconds.

Now - the intention was show the spinner, when the dependent javascript modules gets loaded.
Hence - we did not remove the spinner, but - instead only hide it.

And, changed the text to 'Loading the dependent resources...', so that - whenever we once again show the spinner, it will show that text.
I am considering using the tree events to show the spinner again using the similar approach used in your first patch.

Then - change the feature test to wait for the spinner to go away before expanding the table node.

Regarding your first patch:

Descriptive variable names are clearer than single-letter variable names. Could you clean up the first patch to use clearer variables, perhaps add some tests and break it up into methods so that I can more easily understand what your change does?

Agree.
But - as I said earlier, it represents the data for the tree-item (not the item itself).
Hence -  it should be 'itemData', and not 'item'.

We've already used 'd' to represent the data, 'i' for item, 't' for tree at all other places in pgAdmin 4.
So - it is consistent across the application.

I would add comments to understand the functionality a lot better.

Will share the patch for the same.


-- Thanks, Ashesh

Thank you,

Tira & George


On Mon, Mar 20, 2017 at 8:03 AM, Dave Page <dpage@pgadmin.org> wrote:
On Mon, Mar 20, 2017 at 10:24 AM, Ashesh Vashi
<ashesh.vashi@enterprisedb.com> wrote:
> On Fri, Mar 17, 2017 at 8:35 PM, Sarah McAlear <smcalear@pivotal.io> wrote:
>>
>> Hello,
>>
>> We agree that we should keep an eye on this and the failing feature tests.
>> Our current story touches part of this code, but we won't go into changing
>> the library for now.
>>
>> The patch Tira sent fixes a global variable problem that we found while
>> looking into the code that generates the Tree, that
>> had the potential to load the tree in an infinite loop.
>> Can you apply the patch like this, or would you rather us send it in a
>> separate patch email?
>
> Name of the variable should have been itemData, d (as earlier), as it
> represents the data for the expanding node item.
> And, that is not good enough for resolving the issue.
>
> I've two approaches to resolve the idea.
> 1. Load the nodes (even when the module representing that node is not yet
> loaded)
> Pros:
> - Nodes will be loaded even when the module for the node type is not yet
> loaded.
> Cons:
> - The nodes with modified url (not the default mechanism) may get failed, if
> the module is not yet loaded.
>   (NOTE: We've not no nodes with that changes at the moment. so - we can
> safely ignore it.)
> - Operations specific to the nodes will not be honoured until modules is not
> loaded.
>
> 2. Wait for the modules to get loaded before allowing any operations on
> them.
> Pros:
> - All operations will be done on a node only after the respective module is
> loaded.
> Cons:
> - It will block any operations on pgAdmin 4, when the dependent modules are
> being loaded. It can annoy the user.
>
> Please find the patches for both the approaches.
>
> Dave - please take a look at it.

I'll leave you and Tira to figure this one out if you don't mind. My
plate is kinda full at the moment.

I will note though that neither blocking or potential failures are desirable.

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company





Re: [pgadmin-hackers] Feature test regression failures

От
Ashesh Vashi
Дата:
On Wed, Mar 22, 2017 at 7:23 PM, Atira Odhner <aodhner@pivotal.io> wrote:
 First - let me try to explain the problem with the failure in the feature test.
We do not load all the javascript libraries, when starting the pgAdmin 4 (i.e. the loading the browser/index.html).
But - load them only when first tree-item of certain type is added.
e.g. We load the javascript module of 'table', 'index', etc only after first tree item of database is added in the tree.

I've seen this bug opening the tree by hand, so it is not merely a matter of opening tree items as quickly as a machine. There are two different code paths for loading the tree state --- the way the initial load happens upon expand and the call that is made when the user clicks 'Refresh...' in the context menu. Maybe a good approach to starting to fix this bug would be to have only one code path for loading tree state.
What do you mean by that? 
I don't think this has to do with loading the javascript modules.
It is - we've personally experienced the same in early phase of the development, when the spinner control was not implemented.
We have seen that - when we expand the tree node, it does reloads the server-groups under the server-group node, just because of the same reason.
 
But - as I said earlier, it represents the data for the tree-item (not the item itself).
Hence -  it should be 'itemData', and not 'item'.

'itemData' and 'item' are both fine with me.
 
We've already used 'd' to represent the data, 'i' for item, 't' for tree at all other places in pgAdmin 4.
So - it is consistent across the application.

Consistency is great, but in this case, aiming for consistency is hindering the legibility of the application code. At some point, we should go through the whole application and change all the 'i' to 'item', 'd' to data', etc. Until then, I think it is worthwhile to sacrifice consistency in exchange for adding meaning and clarity to the code that is being updated.
To be honest, not in that case.
As the current choice of the alphabet tells enough about the meaning of the code.

We're deviating from the actual point at the moment, so - let's concentrate on that first.
I would change it to itemData.

-- Thanks, Ashesh

Tira & Joao

On Wed, Mar 22, 2017 at 2:20 AM, Ashesh Vashi <ashesh.vashi@enterprisedb.com> wrote:


--

Thanks & Regards,

Ashesh Vashi
EnterpriseDB INDIA: Enterprise PostgreSQL Company


http://www.linkedin.com/in/asheshvashi


On Tue, Mar 21, 2017 at 9:20 PM, Atira Odhner <aodhner@pivotal.io> wrote:
Here is a new patchset that instead hides the spinner when the acitree has been initialized. On average, the spinner seems to disappear about 2 seconds sooner, and I haven't seen flakiness with these changes yet.

Tira & Joao

On Mon, Mar 20, 2017 at 4:17 PM, Atira Odhner <aodhner@pivotal.io> wrote:
Note that this patch makes the problem of the tree not having loaded worse, because it only waits for js modules to load rather than arbitrarily waiting 900ms.
The patch was never intended to remove the spinner earlier.
Idea was: the spinner should be hidden only after all dependent javascript modules are loaded.

I agree about using arbitrary wait was never a good option for sure.
Though - I still see the flaw in my approach.
If the dependent script has error, it wont get loaded, and can make the things worse by not hiding the spinner at all.

I liked the idea behind your first patch about using the tree events to hide the spinner.
Though - I see scope of improvement in it.

On Mon, Mar 20, 2017 at 3:17 PM, Atira Odhner <aodhner@pivotal.io> wrote:

Hi Ashesh,

Regarding your second patch:

It looks like your second patch addresses module loading. This is an improvement over the previous hard timeout, but won’t do anything for the tree issues. The module loading code can also be simplified; we’ve attached a patchset that is tidier, tests the behavior, and speeds up the polling.

Ashesh, can you explain why you are setting the text on the spinner after hiding it, or why you are hiding it rather than removing it?

First - let me try to explain the problem with the failure in the feature test.
We do not load all the javascript libraries, when starting the pgAdmin 4 (i.e. the loading the browser/index.html).
But - load them only when first tree-item of certain type is added.
e.g. We load the javascript module of 'table', 'index', etc only after first tree item of database is added in the tree.

Now - when we run the feature tests, it expands all the nodes one by one very quickly.
And, that makes the thing worse, as the javascript module for 'table' is still not loaded in the browser, definitely - not immediately after the first 'database' item is added, it always takes some time to load the module, and takes few fraction of seconds/milliseconds.

Now - the intention was show the spinner, when the dependent javascript modules gets loaded.
Hence - we did not remove the spinner, but - instead only hide it.

And, changed the text to 'Loading the dependent resources...', so that - whenever we once again show the spinner, it will show that text.
I am considering using the tree events to show the spinner again using the similar approach used in your first patch.

Then - change the feature test to wait for the spinner to go away before expanding the table node.

Regarding your first patch:

Descriptive variable names are clearer than single-letter variable names. Could you clean up the first patch to use clearer variables, perhaps add some tests and break it up into methods so that I can more easily understand what your change does?

Agree.
But - as I said earlier, it represents the data for the tree-item (not the item itself).
Hence -  it should be 'itemData', and not 'item'.

We've already used 'd' to represent the data, 'i' for item, 't' for tree at all other places in pgAdmin 4.
So - it is consistent across the application.

I would add comments to understand the functionality a lot better.

Will share the patch for the same.


-- Thanks, Ashesh

Thank you,

Tira & George


On Mon, Mar 20, 2017 at 8:03 AM, Dave Page <dpage@pgadmin.org> wrote:
On Mon, Mar 20, 2017 at 10:24 AM, Ashesh Vashi
<ashesh.vashi@enterprisedb.com> wrote:
> On Fri, Mar 17, 2017 at 8:35 PM, Sarah McAlear <smcalear@pivotal.io> wrote:
>>
>> Hello,
>>
>> We agree that we should keep an eye on this and the failing feature tests.
>> Our current story touches part of this code, but we won't go into changing
>> the library for now.
>>
>> The patch Tira sent fixes a global variable problem that we found while
>> looking into the code that generates the Tree, that
>> had the potential to load the tree in an infinite loop.
>> Can you apply the patch like this, or would you rather us send it in a
>> separate patch email?
>
> Name of the variable should have been itemData, d (as earlier), as it
> represents the data for the expanding node item.
> And, that is not good enough for resolving the issue.
>
> I've two approaches to resolve the idea.
> 1. Load the nodes (even when the module representing that node is not yet
> loaded)
> Pros:
> - Nodes will be loaded even when the module for the node type is not yet
> loaded.
> Cons:
> - The nodes with modified url (not the default mechanism) may get failed, if
> the module is not yet loaded.
>   (NOTE: We've not no nodes with that changes at the moment. so - we can
> safely ignore it.)
> - Operations specific to the nodes will not be honoured until modules is not
> loaded.
>
> 2. Wait for the modules to get loaded before allowing any operations on
> them.
> Pros:
> - All operations will be done on a node only after the respective module is
> loaded.
> Cons:
> - It will block any operations on pgAdmin 4, when the dependent modules are
> being loaded. It can annoy the user.
>
> Please find the patches for both the approaches.
>
> Dave - please take a look at it.

I'll leave you and Tira to figure this one out if you don't mind. My
plate is kinda full at the moment.

I will note though that neither blocking or potential failures are desirable.

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company






Re: [pgadmin-hackers] Feature test regression failures

От
Atira Odhner
Дата:
Hi Ashesh,
 
 First - let me try to explain the problem with the failure in the feature test.
We do not load all the javascript libraries, when starting the pgAdmin 4 (i.e. the loading the browser/index.html).
But - load them only when first tree-item of certain type is added.
e.g. We load the javascript module of 'table', 'index', etc only after first tree item of database is added in the tree.

I've seen this bug opening the tree by hand, so it is not merely a matter of opening tree items as quickly as a machine. There are two different code paths for loading the tree state --- the way the initial load happens upon expand and the call that is made when the user clicks 'Refresh...' in the context menu. Maybe a good approach to starting to fix this bug would be to have only one code path for loading tree state.
What do you mean by that? 

I was referring to the fact that fetchNodeInfo gets called when you hit the refresh button in the context menu, but acitree itself is responsible for loading the tree state otherwise. Maybe there is no good way to have a single code path for these two use cases using the acitree library.


I don't think this has to do with loading the javascript modules.
It is - we've personally experienced the same in early phase of the development, when the spinner control was not implemented.
We have seen that - when we expand the tree node, it does reloads the server-groups under the server-group node, just because of the same reason.
 
I understand that the javascript modules are being loaded dynamically when a corresponding item is first revealed in the tree, and although that seems unnecessary, I still don't think that is the cause of this bug. I think it is far more likely to be a result of sharing state that should not be shared.  I am having a hard time tracking down the bug because I find the code very difficult to read largely because of the one-letter variable names and lack of meaningful isolation between code paths.
 

But - as I said earlier, it represents the data for the tree-item (not the item itself).
Hence -  it should be 'itemData', and not 'item'.

'itemData' and 'item' are both fine with me.
 
We've already used 'd' to represent the data, 'i' for item, 't' for tree at all other places in pgAdmin 4.
So - it is consistent across the application.

Consistency is great, but in this case, aiming for consistency is hindering the legibility of the application code. At some point, we should go through the whole application and change all the 'i' to 'item', 'd' to data', etc. Until then, I think it is worthwhile to sacrifice consistency in exchange for adding meaning and clarity to the code that is being updated.
To be honest, not in that case.
As the current choice of the alphabet tells enough about the meaning of the code.

Clearly if I'm renaming it with the wrong thing, it isn't enough for me to tell about the meaning of the code.
 
We're deviating from the actual point at the moment, so - let's concentrate on that first.
I would change it to itemData.
Great. Please change it to itemData.

Thanks, 

Tira & Joao 

Feature test regression failures

От
Dave Page
Дата:
So I had a play with all of these patches. As a sidenote, I'm travelling at the moment, so am using my regular MacBook which is far slower than my normal MacBook Pro. The following is a late-night brain-dump of thoughts...

Tira's various patches all seem to be flakey :-(:

- The spinner is disabled before the top menus are rendered
- If I refresh the page in the browser, and try to expand Servers as quickly as possible after the spinner disappears, I could very easily get it to load another "Servers" node under Servers.

Ashesh's first patch seems to work. Everything loads as it should as far as I can see, and I was unable to reproduce the Servers under Servers issue.

The second patch also seemed to work, but the Loading Resources pause is extremely annoying, though it only seems to happen 3 times until everything is loaded and it's good from there on. That might be less obvious if the treeview spinner was used, and the treeview is disabled (but not greyed) whilst it's spinning.

I do wonder if Tira is correct when she asserts that there's probably no need to delay-load code for individual modules. As I only see the "Loading dependent resources" spinner with Ashesh's 2nd patch twice, I suspect we're loading most things at once anyway.

I also agree that we shouldn't have any shared/global state unnecessarily, and that aciTree's design might be part of the cause of this (though, at the time I first selected it, it seemed to be the best option, as the other tree controls I tried were missing key features). That was a few years ago now though. Perhaps there are better alternatives now, but I shudder to think how much work it might be to swap it out.

I also wonder if this would tie in with some of the code optimisation work I've been tinkering with. I've been looking at having a build step for packages that creates minimised versions of HTML/CSS/JS files, and automatically using those if DEBUG == False and they are present, falling back to un-minimised if required. I do wonder if as part of that minimisation, we could actually combine all these files together, and load everything in one go, saving not just bandwidth but round trips as well.

Anyway, regardless of other things, we still need to get the feature tests working reliably ASAP (Murtuza is working on some more at the moment). Ashesh, please replace the 1 char variable names with something more meaningful as a first step.

Would it help to get the two of you on a call to try to figure this out and agree on a solution? 

Thanks.

On Wednesday, March 22, 2017, Atira Odhner <aodhner@pivotal.io> wrote:
Hi Ashesh,
 
 First - let me try to explain the problem with the failure in the feature test.
We do not load all the javascript libraries, when starting the pgAdmin 4 (i.e. the loading the browser/index.html).
But - load them only when first tree-item of certain type is added.
e.g. We load the javascript module of 'table', 'index', etc only after first tree item of database is added in the tree.

I've seen this bug opening the tree by hand, so it is not merely a matter of opening tree items as quickly as a machine. There are two different code paths for loading the tree state --- the way the initial load happens upon expand and the call that is made when the user clicks 'Refresh...' in the context menu. Maybe a good approach to starting to fix this bug would be to have only one code path for loading tree state.
What do you mean by that? 

I was referring to the fact that fetchNodeInfo gets called when you hit the refresh button in the context menu, but acitree itself is responsible for loading the tree state otherwise. Maybe there is no good way to have a single code path for these two use cases using the acitree library.


I don't think this has to do with loading the javascript modules.
It is - we've personally experienced the same in early phase of the development, when the spinner control was not implemented.
We have seen that - when we expand the tree node, it does reloads the server-groups under the server-group node, just because of the same reason.
 
I understand that the javascript modules are being loaded dynamically when a corresponding item is first revealed in the tree, and although that seems unnecessary, I still don't think that is the cause of this bug. I think it is far more likely to be a result of sharing state that should not be shared.  I am having a hard time tracking down the bug because I find the code very difficult to read largely because of the one-letter variable names and lack of meaningful isolation between code paths.
 

But - as I said earlier, it represents the data for the tree-item (not the item itself).
Hence -  it should be 'itemData', and not 'item'.

'itemData' and 'item' are both fine with me.
 
We've already used 'd' to represent the data, 'i' for item, 't' for tree at all other places in pgAdmin 4.
So - it is consistent across the application.

Consistency is great, but in this case, aiming for consistency is hindering the legibility of the application code. At some point, we should go through the whole application and change all the 'i' to 'item', 'd' to data', etc. Until then, I think it is worthwhile to sacrifice consistency in exchange for adding meaning and clarity to the code that is being updated.
To be honest, not in that case.
As the current choice of the alphabet tells enough about the meaning of the code.

Clearly if I'm renaming it with the wrong thing, it isn't enough for me to tell about the meaning of the code.
 
We're deviating from the actual point at the moment, so - let's concentrate on that first.
I would change it to itemData.
Great. Please change it to itemData.

Thanks, 

Tira & Joao 


--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Feature test regression failures

От
Atira Odhner
Дата:

Tira's various patches all seem to be flakey :-(:

Yes, that's because the way it was working before was to arbitrarily wait a full second. My intent with the patches was not to apply them to the code, but to demonstrate that the issue was not related to time spent loading js files through require.

George and Sarah did a little poking around using aciTree and it seemed like the issue was related to pgadmin code not aciTree code.
Since we are working on re-doing much of the tree in React, it seems like it might make sense to wait on that for trying to solve this issue.

I also wonder if this would tie in with some of the code optimisation work I've been tinkering with. I've been looking at having a build step for packages that creates minimised versions of HTML/CSS/JS files, and automatically using those if DEBUG == False and they are present, falling back to un-minimised if required. I do wonder if as part of that minimisation, we could actually combine all these files together, and load everything in one go, saving not just bandwidth but round trips as well.

Oh, you're working on a build step! Cool! We should probably coordinate since we are also adding a build step in order to bring in React. We are pulling in Grunt and either browserify or webpack. 

Tira

On Sat, Mar 25, 2017 at 10:51 PM, Dave Page <dpage@pgadmin.org> wrote:
So I had a play with all of these patches. As a sidenote, I'm travelling at the moment, so am using my regular MacBook which is far slower than my normal MacBook Pro. The following is a late-night brain-dump of thoughts...

Tira's various patches all seem to be flakey :-(:

- The spinner is disabled before the top menus are rendered
- If I refresh the page in the browser, and try to expand Servers as quickly as possible after the spinner disappears, I could very easily get it to load another "Servers" node under Servers.

Ashesh's first patch seems to work. Everything loads as it should as far as I can see, and I was unable to reproduce the Servers under Servers issue.

The second patch also seemed to work, but the Loading Resources pause is extremely annoying, though it only seems to happen 3 times until everything is loaded and it's good from there on. That might be less obvious if the treeview spinner was used, and the treeview is disabled (but not greyed) whilst it's spinning.

I do wonder if Tira is correct when she asserts that there's probably no need to delay-load code for individual modules. As I only see the "Loading dependent resources" spinner with Ashesh's 2nd patch twice, I suspect we're loading most things at once anyway.

I also agree that we shouldn't have any shared/global state unnecessarily, and that aciTree's design might be part of the cause of this (though, at the time I first selected it, it seemed to be the best option, as the other tree controls I tried were missing key features). That was a few years ago now though. Perhaps there are better alternatives now, but I shudder to think how much work it might be to swap it out.

I also wonder if this would tie in with some of the code optimisation work I've been tinkering with. I've been looking at having a build step for packages that creates minimised versions of HTML/CSS/JS files, and automatically using those if DEBUG == False and they are present, falling back to un-minimised if required. I do wonder if as part of that minimisation, we could actually combine all these files together, and load everything in one go, saving not just bandwidth but round trips as well.

Anyway, regardless of other things, we still need to get the feature tests working reliably ASAP (Murtuza is working on some more at the moment). Ashesh, please replace the 1 char variable names with something more meaningful as a first step.

Would it help to get the two of you on a call to try to figure this out and agree on a solution? 

Thanks.

On Wednesday, March 22, 2017, Atira Odhner <aodhner@pivotal.io> wrote:
Hi Ashesh,
 
 First - let me try to explain the problem with the failure in the feature test.
We do not load all the javascript libraries, when starting the pgAdmin 4 (i.e. the loading the browser/index.html).
But - load them only when first tree-item of certain type is added.
e.g. We load the javascript module of 'table', 'index', etc only after first tree item of database is added in the tree.

I've seen this bug opening the tree by hand, so it is not merely a matter of opening tree items as quickly as a machine. There are two different code paths for loading the tree state --- the way the initial load happens upon expand and the call that is made when the user clicks 'Refresh...' in the context menu. Maybe a good approach to starting to fix this bug would be to have only one code path for loading tree state.
What do you mean by that? 

I was referring to the fact that fetchNodeInfo gets called when you hit the refresh button in the context menu, but acitree itself is responsible for loading the tree state otherwise. Maybe there is no good way to have a single code path for these two use cases using the acitree library.


I don't think this has to do with loading the javascript modules.
It is - we've personally experienced the same in early phase of the development, when the spinner control was not implemented.
We have seen that - when we expand the tree node, it does reloads the server-groups under the server-group node, just because of the same reason.
 
I understand that the javascript modules are being loaded dynamically when a corresponding item is first revealed in the tree, and although that seems unnecessary, I still don't think that is the cause of this bug. I think it is far more likely to be a result of sharing state that should not be shared.  I am having a hard time tracking down the bug because I find the code very difficult to read largely because of the one-letter variable names and lack of meaningful isolation between code paths.
 

But - as I said earlier, it represents the data for the tree-item (not the item itself).
Hence -  it should be 'itemData', and not 'item'.

'itemData' and 'item' are both fine with me.
 
We've already used 'd' to represent the data, 'i' for item, 't' for tree at all other places in pgAdmin 4.
So - it is consistent across the application.

Consistency is great, but in this case, aiming for consistency is hindering the legibility of the application code. At some point, we should go through the whole application and change all the 'i' to 'item', 'd' to data', etc. Until then, I think it is worthwhile to sacrifice consistency in exchange for adding meaning and clarity to the code that is being updated.
To be honest, not in that case.
As the current choice of the alphabet tells enough about the meaning of the code.

Clearly if I'm renaming it with the wrong thing, it isn't enough for me to tell about the meaning of the code.
 
We're deviating from the actual point at the moment, so - let's concentrate on that first.
I would change it to itemData.
Great. Please change it to itemData.

Thanks, 

Tira & Joao 


--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


Re: Feature test regression failures

От
Dave Page
Дата:
On Mon, Mar 27, 2017 at 11:47 AM, Atira Odhner <aodhner@pivotal.io> wrote:
>>
>> Tira's various patches all seem to be flakey :-(:
>
>
> Yes, that's because the way it was working before was to arbitrarily wait a
> full second. My intent with the patches was not to apply them to the code,
> but to demonstrate that the issue was not related to time spent loading js
> files through require.
>
> George and Sarah did a little poking around using aciTree and it seemed like
> the issue was related to pgadmin code not aciTree code.
> Since we are working on re-doing much of the tree in React, it seems like it
> might make sense to wait on that for trying to solve this issue.

A change as large as moving to React will be for v2.x, not v1.x, so we
cannot really wait unless we want to write off the feature tests as
unusable for the forseeable future.

>> I also wonder if this would tie in with some of the code optimisation work
>> I've been tinkering with. I've been looking at having a build step for
>> packages that creates minimised versions of HTML/CSS/JS files, and
>> automatically using those if DEBUG == False and they are present, falling
>> back to un-minimised if required. I do wonder if as part of that
>> minimisation, we could actually combine all these files together, and load
>> everything in one go, saving not just bandwidth but round trips as well.
>
>
> Oh, you're working on a build step! Cool! We should probably coordinate
> since we are also adding a build step in order to bring in React. We are
> pulling in Grunt and either browserify or webpack.

I'm tinkering with ideas for optimising the code we ship to users in
my spare time. This isn't something I'm prioritising. If you're
working on similar ideas, please do share.

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


Re: Feature test regression failures

От
Atira Odhner
Дата:
A change as large as moving to React will be for v2.x, not v1.x, so we
cannot really wait unless we want to write off the feature tests as
unusable for the forseeable future.

I don't think we should wait on moving the entire codebase to react before releasing features that use it. React allows us to implement things piecemeal so for example we can implement this tree in react and ship that as a fully functional user-facing change. Over time we can introduce more components in react while still having a working product.

I'm tinkering with ideas for optimising the code we ship to users in
my spare time. This isn't something I'm prioritising. If you're
working on similar ideas, please do share.

The best practice for using React is to use a preprocessing pipeline so we'll definitely share that as we get it set up. We will definitely be doing minification and bundling code into a single file.

Tira & Matt

On Mon, Mar 27, 2017 at 11:52 AM, Dave Page <dpage@pgadmin.org> wrote:
On Mon, Mar 27, 2017 at 11:47 AM, Atira Odhner <aodhner@pivotal.io> wrote:
>>
>> Tira's various patches all seem to be flakey :-(:
>
>
> Yes, that's because the way it was working before was to arbitrarily wait a
> full second. My intent with the patches was not to apply them to the code,
> but to demonstrate that the issue was not related to time spent loading js
> files through require.
>
> George and Sarah did a little poking around using aciTree and it seemed like
> the issue was related to pgadmin code not aciTree code.
> Since we are working on re-doing much of the tree in React, it seems like it
> might make sense to wait on that for trying to solve this issue.

A change as large as moving to React will be for v2.x, not v1.x, so we
cannot really wait unless we want to write off the feature tests as
unusable for the forseeable future.

>> I also wonder if this would tie in with some of the code optimisation work
>> I've been tinkering with. I've been looking at having a build step for
>> packages that creates minimised versions of HTML/CSS/JS files, and
>> automatically using those if DEBUG == False and they are present, falling
>> back to un-minimised if required. I do wonder if as part of that
>> minimisation, we could actually combine all these files together, and load
>> everything in one go, saving not just bandwidth but round trips as well.
>
>
> Oh, you're working on a build step! Cool! We should probably coordinate
> since we are also adding a build step in order to bring in React. We are
> pulling in Grunt and either browserify or webpack.

I'm tinkering with ideas for optimising the code we ship to users in
my spare time. This isn't something I'm prioritising. If you're
working on similar ideas, please do share.

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Feature test regression failures

От
Dave Page
Дата:
On Mon, Mar 27, 2017 at 12:05 PM, Atira Odhner <aodhner@pivotal.io> wrote:
>> A change as large as moving to React will be for v2.x, not v1.x, so we
>> cannot really wait unless we want to write off the feature tests as
>> unusable for the forseeable future.
>
>
> I don't think we should wait on moving the entire codebase to react before
> releasing features that use it. React allows us to implement things
> piecemeal so for example we can implement this tree in react and ship that
> as a fully functional user-facing change. Over time we can introduce more
> components in react while still having a working product.

Maybe - I'd need to see a viable patch with minimal impact first though.

We've already broken many of the normal rules we play by with pgAdmin
4 by cutting releases from the master branch rather than keeping a
stable branch with only bug fixes in it, and I'm extremely wary about
shipping largely non-essential changes in a minor update without being
able to do the level of QA we would for a major release. The risk to
the user and to our reputation is too high.

>> I'm tinkering with ideas for optimising the code we ship to users in
>> my spare time. This isn't something I'm prioritising. If you're
>> working on similar ideas, please do share.
>
>
> The best practice for using React is to use a preprocessing pipeline so
> we'll definitely share that as we get it set up. We will definitely be doing
> minification and bundling code into a single file.

That'll only work to a point. What happens if the user installs a
plugin to get some additional functionality? We still need the ability
to ensure code can be added to an existing installation.

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


Re: Feature test regression failures

От
Atira Odhner
Дата:

Maybe - I'd need to see a viable patch with minimal impact first though.

Sure, makes sense.

That'll only work to a point. What happens if the user installs a
plugin to get some additional functionality? We still need the ability
to ensure code can be added to an existing installation.

We can definitely set it up where that won't be an issue. It will probably be up to the plugin developer to do minification of the plugin's javascript, or the plugin javascript can simply work unminified.




On Mon, Mar 27, 2017 at 12:14 PM, Dave Page <dpage@pgadmin.org> wrote:
On Mon, Mar 27, 2017 at 12:05 PM, Atira Odhner <aodhner@pivotal.io> wrote:
>> A change as large as moving to React will be for v2.x, not v1.x, so we
>> cannot really wait unless we want to write off the feature tests as
>> unusable for the forseeable future.
>
>
> I don't think we should wait on moving the entire codebase to react before
> releasing features that use it. React allows us to implement things
> piecemeal so for example we can implement this tree in react and ship that
> as a fully functional user-facing change. Over time we can introduce more
> components in react while still having a working product.

Maybe - I'd need to see a viable patch with minimal impact first though.

We've already broken many of the normal rules we play by with pgAdmin
4 by cutting releases from the master branch rather than keeping a
stable branch with only bug fixes in it, and I'm extremely wary about
shipping largely non-essential changes in a minor update without being
able to do the level of QA we would for a major release. The risk to
the user and to our reputation is too high.

>> I'm tinkering with ideas for optimising the code we ship to users in
>> my spare time. This isn't something I'm prioritising. If you're
>> working on similar ideas, please do share.
>
>
> The best practice for using React is to use a preprocessing pipeline so
> we'll definitely share that as we get it set up. We will definitely be doing
> minification and bundling code into a single file.

That'll only work to a point. What happens if the user installs a
plugin to get some additional functionality? We still need the ability
to ensure code can be added to an existing installation.

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company