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 1f4N3H-0000zm-1R for pgadmin-hackers@arkaria.postgresql.org; Fri, 06 Apr 2018 08:48: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 1f4N3F-0001Ka-Dt for pgadmin-hackers@arkaria.postgresql.org; Fri, 06 Apr 2018 08:48:53 +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 1f4N3F-0001KP-7J for pgadmin-hackers@lists.postgresql.org; Fri, 06 Apr 2018 08:48:53 +0000 Received: from mail-oi0-x229.google.com ([2607:f8b0:4003:c06::229]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1f4N35-0008Di-SO for pgadmin-hackers@postgresql.org; Fri, 06 Apr 2018 08:48:52 +0000 Received: by mail-oi0-x229.google.com with SMTP id 126-v6so376904oig.0 for ; Fri, 06 Apr 2018 01:48:43 -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=SKLDzjb3G8Tj1+jzL+NXJyuXGi4z05uMZxrWa5z1cbY=; b=NfC+OuYF2yd1BV00eY3fZPkLFJu+DWj4MVnqL52LfMhVPoOj8YYuXzYuP8R2xL7juC T7Q8ZOwYdxD9Ay1lqQ87Skeh2sz9XMmF/CKodWrX7EeQIw7vXslghIooxVUQU3RJidyp al1hrBJEXo3tMaFQ9PAZU+Xu4vsR0qxLbfFK7Ky7CC3Hg4D95sB9CJtxBIomNF86zJaX vv/heD0FZ4O9t2oLhTEhHiyJqhfq7o0k+Fnjgyqq0YT1chGoSTgdP4wnXSeZMpox70iG /OXhmMTYC1k8uqcdGNFudRqZHdTna6eevXbBvN5psBo3exl4fsNSeXQIxoFUonwX3gRX F5MQ== 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=SKLDzjb3G8Tj1+jzL+NXJyuXGi4z05uMZxrWa5z1cbY=; b=AVixMcOgYPUIXcBq+C155F3Q7Yw/cj4VBYYHd9MRZH5TjCv97/jP496B2y92wbiAoc fJ9AqjFSyPA/+dEm6D/Dd4PCvCtJ6dVdB9hCyfeZLCWwnn1yjH11b9U3S8tzI/2EvXjY OJvkv+2yLLc5oGrmC117N7kslal9VExJNnUz1bBmR3qg42yK5OBi0HMNthx1SpFU7NBr 8+AkqX2YSdEgniAmSKDGhZo0phUqzclYuy5Xu0hUdaOTCS1H9eRNpQ43ITJpValGPG+2 zU9cqqiFasgFTzuBInPqu7xLMdbIGTSLR+s8M4Uog1gl3I6c+0inLeCW0YoSVB+5wqgX m2gg== X-Gm-Message-State: ALQs6tBa5jtpywfbpdkzwAIshWcV8hSZEaowrA9u9BfiaHqaiEudYHuQ Lrn67vEsCTHaP/zU43GV+xEAV/VhL7LSLHydqTpMtQ== X-Google-Smtp-Source: AIpwx49rgBwXrseXRTf+sdoxzA4ucCEvTkQ4O0U9iKCvsQLWqTm10e2x4t6NvaEmvy0jJrUPK5b2H3jl6mSIswWky+k= X-Received: by 2002:aca:4a15:: with SMTP id x21-v6mr15471015oia.211.1523004521607; Fri, 06 Apr 2018 01:48:41 -0700 (PDT) MIME-Version: 1.0 Received: by 10.138.6.138 with HTTP; Fri, 6 Apr 2018 01:48:21 -0700 (PDT) In-Reply-To: References: From: Murtuza Zabuawala Date: Fri, 6 Apr 2018 14:18:21 +0530 Message-ID: Subject: Re: [pgadmin4][patch] Initial patch to decouple from ACI Tree To: Joao De Almeida Pereira Cc: Khushboo Vashi , pgadmin-hackers Content-Type: multipart/alternative; boundary="00000000000080894405692a1f62" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --00000000000080894405692a1f62 Content-Type: text/plain; charset="UTF-8" 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 code 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 the new tree >>> behavior on our tests >>> - Adds tests for all the tree functionalities >>> >>> 0002 patch: >>> - Extracts, refactors, adds tests and remove dependency from ACI Tree >>> 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. From >>> 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 renamed 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 occurrences of >>> "itemData" in the code and roughly 176 calls to the function itself (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, 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 >>> 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 to >>> use only with ACITree. >>> >>> >>> Thanks >>> Joao >>> >>> >> --00000000000080894405692a1f62 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
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 he= lp us to understand the flow, I'm saying because now the code is segreg= ated in so many separate files it will be hard to keep track of the flow fr= om one file to another when debugging.


<= div>
--
Regards,
Murtuza Zabuawala
EnterpriseDB:=C2=A0http://www.en= terprisedb.com
The Enterprise PostgreSQL Company
=
=

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

Thanks
<= div>

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 int= ended to be merged after 3.0, because we do not want to cause any entropy o= r 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 th= e community to do it.

Summary of the patch:=
0001 patch:=C2=A0
=C2=A0- Creates a new tree that will= allow us to create a separation between the application and ACI Tree
=
=C2=A0- Creates a Fake Tree (Test double, for reference on the availab= le test doubles:=C2=A0https://martinfowler.com/bliki/TestDouble.html= ) that can be used to inplace to replace the ACITree and also encapsula= te the new tree behavior on our tests
=C2=A0- Adds tests for all = the tree functionalities

0002 patch:
=C2= =A0- Extracts, refactors, adds tests and remove dependency from ACI Tree on= :
- getTreeNodeHierarchy
- on backup.js: menu_enabled, menu= _enabled_server, start_backup_global_server, backup_objects
- on dat= agrid.js: show_data_grid, get_panel_title, show_query_tool
- Start usin= g sprintf-js as Underscore.String is deprecating sprintf function
= =C2=A0=C2=A0
This patch represents only 10 calls to ACITree.itemD= ata 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 suppor= ted, the documentation is non existent and ACITree is no longer being activ= ely developed.

Our process:
1. We "= randomly" selected a function that is part of the ACITree. From this p= oint we decided to replace that function with our own version. The function= that we choose was "itemData".
The function gives us a= ll the "data" that a specific node of the tree find.
Gi= ven in order to replace the tree we would need to have a function that woul= d give us the same information. We had 2 options:
=C2=A0 a) Creat= e a tree with a function called itemData
Pros:
=C2=A0- = At first view this was the simpler solution
=C2=A0- Would keep the sta= tus quo
Cons:
=C2=A0- Not a OOP approach
=C2=A0- No= t very flexible
=C2=A0 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:=C2=A0
=C2=A0- OOP Approach
=C2=A0- Mo= re flexible and we do not need to bring the tree around, just a node
<= div>Cons:
=C2=A0- Break the current status quo

Given these 2 options we decided to go for a more OOP approach creat= ing a Tree and a TreeNode classes, that in the future will be renamed to AC= ITreeWrapper and TreeNode.

2. After we decided on = the starting point we searched for occurrences of the function "itemDa= ta" and we found out that there were 303 occurrences of "itemData= " in the code and roughly 176 calls to the function itself (some of th= e hits were variable names).

3. We selected the fi= rst file on the search and found the function that was responsible for call= ing the itemData function.

4. Extracted the functi= on to a separate file

5. Wrap this function with t= ests

6. Refactor the function to ES6, give more de= clarative names to variables and break the functions into smaller chunks

7. When all the tests were passing we replaced ACITr= ee 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 st= ill works

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

As you can see by the process this is a pretty huge u= ndertake, 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 problem with a large number of elements on the tree.

What is on our radar that we need to address:
=
=C2=A0- Finish the complete decoupling of the ACITree
=C2=A0= - Performance of the current tree implementation
=C2=A0- Tweak th= e naming of the Tree class to explicitly tell us this is to use only with A= CITree.


Thanks
<= div>Joao



--00000000000080894405692a1f62--