Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1f5PqN-00014R-IN for pgadmin-hackers@arkaria.postgresql.org; Mon, 09 Apr 2018 05:59:55 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1f5PqK-0005ev-U0 for pgadmin-hackers@arkaria.postgresql.org; Mon, 09 Apr 2018 05:59:52 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1f5PqK-0005el-JY for pgadmin-hackers@lists.postgresql.org; Mon, 09 Apr 2018 05:59:52 +0000 Received: from mail-oi0-x22a.google.com ([2607:f8b0:4003:c06::22a]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1f5PqA-0002Kq-6g for pgadmin-hackers@postgresql.org; Mon, 09 Apr 2018 05:59:51 +0000 Received: by mail-oi0-x22a.google.com with SMTP id u141-v6so6507901oif.1 for ; Sun, 08 Apr 2018 22:59:41 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=enterprisedb-com.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=4ze47IYlHaYOHamZ+EwY2PcDpnOF1QLEuQhxYz7HLv0=; b=ryPubqIEo+kmsJy+Gx6cmuRRHZK/xwfrWleZo+iuPGxuvRbezo/CVQl+a0QKbptd7F vZgZwFzN9472cTBaWKEIr0r1tba6qsmWrBmSU/iRWTRboumNnIPtvjwbycP/ac7F+YbZ hXgbspCmWbtL3S5y1487cNEZ7X3yzyDPnD2y3HadjsHiaoS+VaQ82883GniZ/YVr0zka /VAFGtcWLDHHJdpcsQF1soVMgF7QTm3i4sk1yhS6EREuGgOSGdYwoWKIKk1TGypaxMyc NnpTUCBwjrQ8jU+IGTWPt5kO+eehvyERxlZ4lYzaFbu7KwoI2onpB4o1IqOP1PcRQjNL zjkA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=4ze47IYlHaYOHamZ+EwY2PcDpnOF1QLEuQhxYz7HLv0=; b=kySRPRWik5dyH7kcq++hrClO/jS0Y5Tu5LsZTKTn0LkIrQc7FJImc3v+kJSo8TdqbT lrgJgM0JD08jj0EKmYWeegFIvEK9Ez9ouj5+TxfaYy6MYXrKtdct+WvX7yzbZrD/ni0k paKRyU9QFUdUOtVxu/hCNms5qHuYeffdaNl7dGodGEVjchaJidkieyqf5fNHcA0yLnQ7 fCtRn1524LC0uEiDk3Cu7bZbXTiNQDetiOwR0YOBxrjmOUXMQMr+LXvGYsDevaYBpAKh hSFpYMFNIQI45C5UuHtDPki7UdEIy7lXGFO7UXj2jMhx+aZc2sivKtrfh1pbJJvug/+C z7cw== X-Gm-Message-State: ALQs6tDTbi5Tk8XZ5Z43nVEwxhlIQeO+Pzr4ZGsK6UBKcsr3Vx5Xj0GV GRS0KmtC9mpGcLWnpfKJfNoH7Nl5teHNQD+NMYTSRw== X-Google-Smtp-Source: AIpwx4+c5Jc6E6I5cKUekZncEiUBhwMZNTzcnKc1I8LW3I3RsAK8+6Qk1OfLimfuvk7ie9W6/tm6IKArOsoQZaoeD5A= X-Received: by 2002:aca:d455:: with SMTP id l82-v6mr22189546oig.102.1523253579570; Sun, 08 Apr 2018 22:59:39 -0700 (PDT) MIME-Version: 1.0 Received: by 10.74.138.251 with HTTP; Sun, 8 Apr 2018 22:59:38 -0700 (PDT) In-Reply-To: References: From: Khushboo Vashi Date: Mon, 9 Apr 2018 11:29:38 +0530 Message-ID: Subject: Re: [pgadmin4][patch] Initial patch to decouple from ACI Tree To: Joao De Almeida Pereira Cc: Murtuza Zabuawala , pgadmin-hackers Content-Type: multipart/alternative; boundary="0000000000008376a00569641cb4" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --0000000000008376a00569641cb4 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi Joao, I have reviewed your patch and have some suggestions. On Sat, Apr 7, 2018 at 12:43 AM, Joao De Almeida Pereira < jdealmeidapereira@pivotal.io> wrote: > Hello Murtuza/Dave, > Yes now the extracted functions are spread into different files. The > intent would be to make the files as small as possible, and also to group > and name them in a way that would be easy to understand what each file is > doing without the need of opening it. > As a example: > static/js/backup will contain all the backup related functionality inside > of this folder we can see the file: > menu_utils.js At this moment in time we decided to group all the functions > that are related to the menu, but we can split that also if we believe it > is easier to see. > It's really very good to see the separated code for backup module. As we have done for backup, we would like do it for other PG utilities like restore, maintenance etc. Considering this, we should separate the code in a way that some of the common functionalities can be used for other modules like menu (as you have mentioned above), dialogue factory etc. Also, I think these functionalities should be in their respective static folder instead of pgadmin/static. > static/js/datagrid folder contains all the datagrid related functionality > Same as backup module, this should be in it's respective static/js folder. > Inside of the folder we can see the files: > get_panel_title.js is responsible for retrieving the name of the panel > show_data.js is responsible for showing the datagrid > show_query_tool.js is responsible for showing the query tool > > Does this structure make sense? > Can you give an example of a comment that you think is missing and that > could help? > > As a personal note, unless the algorithm is very obscure or very > complicated, I believe that if the code needs comments it is a signal tha= t > something needs to change in terms of naming, structure of the part in > question. This being said, I am open to add some comments that might help > people. > You are right, with the help of naming convention and structure of the code, any one can get the idea about the code. But it is very useful to understand the code very easily with the proper comments especially when there are multiple developers working on a single project. I found some of the places where it would be great to have comments. - treeMenu: new tree.Tree() in a browser.js - tree.js (especially Tree class) Thanks > Joao > =E2=80=8B > > Thanks, Khushboo > > On Fri, Apr 6, 2018 at 4:48 AM Murtuza Zabuawala < > murtuza.zabuawala@enterprisedb.com> wrote: > >> Hi Joao, >> >> Patch looks good and working as expected. >> >> I also agree with Dave, Can we please add some comments in each file >> which can help us to understand the flow, I'm saying because now the cod= e >> is segregated in so many separate files it will be hard to keep track of >> the flow from one file to another when debugging. >> >> >> -- >> Regards, >> Murtuza Zabuawala >> EnterpriseDB: http://www.enterprisedb.com >> The Enterprise PostgreSQL Company >> >> >> On Thu, Apr 5, 2018 at 7:08 PM, Joao De Almeida Pereira < >> jdealmeidapereira@pivotal.io> wrote: >> >>> Hi Khushboo, >>> Attached you can find both patches rebased >>> >>> Thanks >>> >>> >>> On Thu, Apr 5, 2018 at 6:31 AM Khushboo Vashi < >>> khushboo.vashi@enterprisedb.com> wrote: >>> >>>> Hi Joao, >>>> >>>> Can you please rebase the second patch? >>>> >>>> Thanks, >>>> Khushboo >>>> >>>> >>>> On Tue, Apr 3, 2018 at 12:15 AM, Joao De Almeida Pereira < >>>> jdealmeidapereira@pivotal.io> wrote: >>>> >>>>> Hi Hackers, >>>>> >>>>> Attached you can find the patch that will start to decouple pgAdmin >>>>> from ACITree library. >>>>> This patch is intended to be merged after 3.0, because we do not want >>>>> to cause any entropy or delay the release, but we want to start the >>>>> discussion and show some code. >>>>> >>>>> This job that we started is a massive tech debt chore that will take >>>>> some time to finalize and we would love the help of the community to = do it. >>>>> >>>>> *Summary of the patch:* >>>>> 0001 patch: >>>>> - Creates a new tree that will allow us to create a separation >>>>> between the application and ACI Tree >>>>> - Creates a Fake Tree (Test double, for reference on the available >>>>> test doubles: https://martinfowler.com/bliki/TestDouble.html) that >>>>> can be used to inplace to replace the ACITree and also encapsulate th= e new >>>>> tree behavior on our tests >>>>> - Adds tests for all the tree functionalities >>>>> >>>>> 0002 patch: >>>>> - Extracts, refactors, adds tests and remove dependency from ACI Tre= e >>>>> on: >>>>> - getTreeNodeHierarchy >>>>> - on backup.js: menu_enabled, menu_enabled_server, >>>>> start_backup_global_server, backup_objects >>>>> - on datagrid.js: show_data_grid, get_panel_title, show_query_tool >>>>> - Start using sprintf-js as Underscore.String is deprecating sprintf >>>>> function >>>>> >>>>> This patch represents only 10 calls to ACITree.itemData out of 176 >>>>> that are spread around our code >>>>> >>>>> >>>>> *In Depth look on the process behind the patch:* >>>>> >>>>> We started writing this patch with the idea that we need to decouple >>>>> pgAdmin4 from ACITree, because ACITree is no longer supported, the >>>>> documentation is non existent and ACITree is no longer being actively >>>>> developed. >>>>> >>>>> Our process: >>>>> 1. We "randomly" selected a function that is part of the ACITree. Fro= m >>>>> this point we decided to replace that function with our own version. = The >>>>> function that we choose was "itemData". >>>>> The function gives us all the "data" that a specific node of the tree >>>>> find. >>>>> Given in order to replace the tree we would need to have a function >>>>> that would give us the same information. We had 2 options: >>>>> a) Create a tree with a function called itemData >>>>> Pros: >>>>> - At first view this was the simpler solution >>>>> - Would keep the status quo >>>>> Cons: >>>>> - Not a OOP approach >>>>> - Not very flexible >>>>> b) Create a tree that would return a node given an ID and then the >>>>> node would be responsible for giving it's data. >>>>> Pros: >>>>> - OOP Approach >>>>> - More flexible and we do not need to bring the tree around, just a >>>>> node >>>>> Cons: >>>>> - Break the current status quo >>>>> >>>>> Given these 2 options we decided to go for a more OOP approach >>>>> creating a Tree and a TreeNode classes, that in the future will be re= named >>>>> to ACITreeWrapper and TreeNode. >>>>> >>>>> 2. After we decided on the starting point we searched for occurrences >>>>> of the function "itemData" and we found out that there were 303 occur= rences >>>>> of "itemData" in the code and roughly 176 calls to the function itsel= f >>>>> (some of the hits were variable names). >>>>> >>>>> 3. We selected the first file on the search and found the function >>>>> that was responsible for calling the itemData function. >>>>> >>>>> 4. Extracted the function to a separate file >>>>> >>>>> 5. Wrap this function with tests >>>>> >>>>> 6. Refactor the function to ES6, give more declarative names to >>>>> variables and break the functions into smaller chunks >>>>> >>>>> 7. When all the tests were passing we replaced ACITree with our Tree >>>>> >>>>> 8. We ensured that all tests were passing >>>>> >>>>> 9. Remove function from the original file and use the new function >>>>> >>>>> 10. Ensure everything still works >>>>> >>>>> 11. Find the next function and execute from step 4 until all the >>>>> functions are replaced, refactored and tested. >>>>> >>>>> As you can see by the process this is a pretty huge undertake, becaus= e >>>>> of the number of calls to the function. This is just the first step o= n the >>>>> direction of completely isolating the ACITree so that we can solve th= e >>>>> problem with a large number of elements on the tree. >>>>> >>>>> *What is on our radar that we need to address:* >>>>> - Finish the complete decoupling of the ACITree >>>>> - Performance of the current tree implementation >>>>> - Tweak the naming of the Tree class to explicitly tell us this is t= o >>>>> use only with ACITree. >>>>> >>>>> >>>>> Thanks >>>>> Joao >>>>> >>>>> >>>> >> --0000000000008376a00569641cb4 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi Joao,

I have reviewed= your patch and have some suggestions.

=
On Sat, Apr 7, 2018 at 12:43 AM, Joao De Almeida= Pereira <jdealmeidapereira@pivotal.io> wrote:

Hello Murtuza/Dave,
Yes now the ex= tracted functions are spread into different files. The intent would be to m= ake the files as small as possible, and also to group and name them in a wa= y that would be easy to understand what each file is doing without the need= of opening it.
As a example:
static/js/backup will contain all the backup related functionality inside of this folder w= e can see the file:

menu_utils.js At this moment in time we d= ecided to group all the functions that are related to the menu, but we can = split that also if we believe it is easier to see.

It's really very good to see the separated code for backup modu= le. As we have done for backup, we would like do it for other PG utilities = like restore, maintenance etc.
Considering this, we should separate the= code in a way that some of the common functionalities can be used for othe= r modules=C2=A0 like menu (as you have mentioned above), dialogue factory e= tc.
Also, I think these functionalities should be in their respective s= tatic folder instead of pgadmin/static.
=C2=A0

static/js/datagrid folder contains all the datagrid related functionality

Same as backup module,=C2=A0 this should be in it's = respective static/js folder.

In= side of the folder we can see the files:
get_panel_titl= e.js is responsible for retrieving the name of the panel
show_data.js is responsible for showing the datagrid
show_query_tool.js is responsible for showing the quer= y tool

Does this structure make sense?
Can yo= u give an example of a comment that you think is missing and that could hel= p?

As a personal note, unless the algorithm = is very obscure or very complicated, I believe that if the code needs comme= nts it is a signal that something needs to change in terms of naming, struc= ture of the part in question. This being said, I am open to add some commen= ts that might help people.

You are right, = with the help of naming convention and structure of the code, any one can g= et the idea about the code. But it is very useful to understand the code=C2= =A0
very easily with the proper comments especially when there ar= e multiple developers working on a single project.

I found some of the places where it would be great to have comments.
=

- treeMenu: new tree.Tree()=C2=A0 in a browser.js
=
- tree.js=C2=A0 (especially Tree class)

Thanks
Joao

=E2=80=8B

Thanks,
Khushboo=C2=A0

On Fri, Apr= 6, 2018 at 4:48 AM Murtuza Zabuawala <murtuza.zabuawala@enterprisedb.= com> wrote:
Hi Joao,

Patch looks good and working as expected.

I also agree with = Dave, Can we please add some comments in each file which can help us to und= erstand the flow, I'm saying because now the code is segregated in so m= any separate files it will be hard to keep track of the flow from one file = to another when debugging.


=
--
Regards,
Murtuza Zabuawala
EnterpriseDB:=C2=A0http://w= ww.enterprisedb.com
The Enterprise PostgreSQL Company


On Thu, Apr 5, 2018 at 7:08 PM, Joao De Alme= ida Pereira <jdealmeidapereira@pivotal.io> wrote:=
Hi K= hushboo,
Attached you can find both patches rebased

Thanks


On Thu, Apr 5, 2018 at 6:31 AM Khushboo Vashi <khushboo.vashi@enterprise= db.com> wrote:
Hi Joao,

Can you please reba= se the second patch?

Thanks,
Khushboo


On Tue, Apr 3, 2018 at 12:15 AM, Joao De Almeida Pereira = <jdealmeidapereira@pivotal.io> wrote:
Hi Hackers,
=
Attached you can find the patch that will start to decouple = pgAdmin from ACITree library.
This patch is intended to be merged= after 3.0, because we do not want to cause any entropy or delay the releas= e, but we want to start the discussion and show some code.

This job that we started is a massive tech debt chore that will ta= ke some time to finalize and we would love the help of the community to do = it.

Summary of the patch:
0001 pa= tch:=C2=A0
=C2=A0- Creates a new tree that will allow us to creat= e a separation between the application and ACI Tree
=C2=A0- Creat= es a Fake Tree (Test double, for reference on the available test doubles:= =C2=A0https://martinfowler.com/bliki/TestDouble.html) that can b= e used to inplace to replace the ACITree and also encapsulate the new tree = behavior on our tests
=C2=A0- Adds tests for all the tree functio= nalities

0002 patch:
=C2=A0- Extracts, r= efactors, adds tests and remove dependency from ACI Tree on:
- getT= reeNodeHierarchy
- on backup.js: menu_enabled, menu_enabled_server,= start_backup_global_server, backup_objects
- on datagrid.js: show_d= ata_grid, get_panel_title, show_query_tool
- Start using sprintf-js as = Underscore.String is deprecating sprintf function
=C2=A0=C2=A0
This patch represents only 10 calls to ACITree.itemData out of 176 th= at are spread around our code


In Depth look on the process behind the patch:

We started writing this patch with the idea that we need to decouple = pgAdmin4 from ACITree, because ACITree is no longer supported, the document= ation is non existent and ACITree is no longer being actively developed.

Our process:
1. We "randomly" se= lected a function that is part of the ACITree. From this point we decided t= o replace that function with our own version. The function that we choose w= as "itemData".
The function gives us all the "data= " that a specific node of the tree find.
Given in order to r= eplace the tree we would need to have a function that would give us the sam= e information. We had 2 options:
=C2=A0 a) Create a tree with a f= unction called itemData
Pros:
=C2=A0- At first view thi= s was the simpler solution
=C2=A0- Would keep the status quo
Cons:=
=C2=A0- Not a OOP approach
=C2=A0- Not very flexible
=C2=A0 b) Create a tree that would return a node given an ID and t= hen the node would be responsible for giving it's data.
Pros:= =C2=A0
=C2=A0- OOP Approach
=C2=A0- More flexible and w= e do not need to bring the tree around, just a node
Cons:
=C2=A0- Break the current status quo

Given thes= e 2 options we decided to go for a more OOP approach creating a Tree and a = TreeNode classes, that in the future will be renamed to ACITreeWrapper and = TreeNode.

2. After we decided on the starting poin= t we searched for occurrences of the function "itemData" and we f= ound out that there were 303 occurrences of "itemData" in the cod= e and roughly 176 calls to the function itself (some of the hits were varia= ble names).

3. We selected the first file on the s= earch and found the function that was responsible for calling the itemData = function.

4. Extracted the function to a separate = file

5. Wrap this function with tests
6. Refactor the function to ES6, give more declarative names t= o variables and break the functions into smaller chunks

7. When all the tests were passing we replaced ACITree with our Tree<= /div>

8. We ensured that all tests were passing

9. Remove function from the original file and use the new = function

10. Ensure everything still works

11. Find the next function and execute from step 4 until = all the functions are replaced, refactored and tested.

=
As you can see by the process this is a pretty huge undertake, because= of the number of calls to the function. This is just the first step on the= direction of completely isolating the ACITree so that we can solve the pro= blem with a large number of elements on the tree.

= What is on our radar that we need to address:
=C2=A0- Fini= sh the complete decoupling of the ACITree
=C2=A0- Performance of = the current tree implementation
=C2=A0- Tweak the naming of the T= ree class to explicitly tell us this is to use only with ACITree.


Thanks
Joao



--0000000000008376a00569641cb4--