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 1f3vgo-0000cS-BP for pgadmin-hackers@arkaria.postgresql.org; Thu, 05 Apr 2018 03:35:54 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1f3vgn-0002WJ-62 for pgadmin-hackers@arkaria.postgresql.org; Thu, 05 Apr 2018 03:35: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 1f3vgm-0002W9-VW for pgadmin-hackers@lists.postgresql.org; Thu, 05 Apr 2018 03:35:53 +0000 Received: from mail-ot0-x234.google.com ([2607:f8b0:4003:c0f::234]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1f3vgd-0001rn-SA for pgadmin-hackers@postgresql.org; Thu, 05 Apr 2018 03:35:52 +0000 Received: by mail-ot0-x234.google.com with SMTP id 23-v6so25739479otj.0 for ; Wed, 04 Apr 2018 20:35: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=HM/++3UApJ8NhBcH5vF/ppdOt6nhIwXxoPJoEIH2v7c=; b=ke8AZfcNJE5tS39h+t2c06G9ytkZjLkOTIS/jS3o3tJXrKfBWMYhgC72DMsXykFeC1 af7VhFaTAXt9GxAqoTCiNpXVOTBaLyOU7/XLLs1Imh9BIRM7DiGhiqwIn+ZsJ9CSnuCA JRd0OX0OTPLKdvHVFFW4qn0JjZu1BMtSkcrtKf2YVqbgPC6yhYDfE/wdIcjUoZ8Fg264 dUX6STAFACOx7wkNtZGHVHsHdhFceUBSxgZYhvT3BqyvJWNH0LpSdyidNi9fU+xKG7HE lKU96c9wYXBbHLVKzgZfSdKGabNSQTmA5vGaTssaTRuMMvCI9+vzVeRpSCXyiaGawOf+ PXdA== 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=HM/++3UApJ8NhBcH5vF/ppdOt6nhIwXxoPJoEIH2v7c=; b=jIEBXIqr9/oK8IbtpOXRyHQ2jya6zOHezOLaC1XKVK9XZDHB0pW08RoKVvDPn6SMX5 1fWq5mNbBDe9PT7wErk2rKKjxo/6WwImcuUkj80LxWtXZIFOIYuxuUqOmNkJ+G9BRcqv /wvuUuo1NGjnyz7jThtXDS1m5Hacmo8KBUSKrfaBb30BojWDDR66z6zD1rDNY3Dk4kRu BPjcwpBHMH/A9UG/PfGdmtVXaV2ArAC/twgMdJjlSk7KwPRgPUGGa4FyDndrzlC99/6M 35fD4GRorcWd7cqrLyTJabl6t5gn0JIFP31fuHZF1ejmfnzT0dfS8XQ9TpgPBgWoM/Vn x5kg== X-Gm-Message-State: ALQs6tCJC23OOZC1dVP2FpWmuwYnGE0KzQdbvUjTtWNIdUN77o6Dqi+0 pEn7Rw9xKMM3RAXSOZBiHLkm7C5a/h5ffIiw0yH0XA== X-Google-Smtp-Source: AIpwx49XSJoeAMEUU/W2nQiqYd+73EgVSbBABU2y81gRk7oMtzwK14AqU7s5czWgl0/VhUX0EXVknu8YhhwEQJWbbnQ= X-Received: by 2002:a9d:58ca:: with SMTP id s10-v6mr11635934oth.275.1522899340972; Wed, 04 Apr 2018 20:35:40 -0700 (PDT) MIME-Version: 1.0 Received: by 10.74.138.251 with HTTP; Wed, 4 Apr 2018 20:35:40 -0700 (PDT) In-Reply-To: References: From: Khushboo Vashi Date: Thu, 5 Apr 2018 09:05:40 +0530 Message-ID: Subject: Re: [pgadmin4][patch] Initial patch to decouple from ACI Tree To: Dave Page Cc: Joao De Almeida Pereira , pgadmin-hackers , Murtuza Zabuawala Content-Type: multipart/alternative; boundary="0000000000003f72f9056911a2b2" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --0000000000003f72f9056911a2b2 Content-Type: text/plain; charset="UTF-8" On Wed, Apr 4, 2018 at 9:47 PM, Dave Page wrote: > Khushboo, Murtuza, > > Can you spend some time reviewing this please? I've started playing with > it as well - the first thing that's irking me somewhat is the lack of > comments. Descriptive function names are all well and good, but sometimes a > little more is needed, especially for less experienced developers or > newcomers to the application! > > Sure, already in our list. > On Mon, Apr 2, 2018 at 7:45 PM, 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 >> >> > > > -- > Dave Page > Blog: http://pgsnake.blogspot.com > Twitter: @pgsnake > > EnterpriseDB UK: http://www.enterprisedb.com > The Enterprise PostgreSQL Company > --0000000000003f72f9056911a2b2 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


On Wed, Apr 4, 2018 at 9:47 PM, Dave Page <dpage@pgadmin.org> wrote:
Khushboo, M= urtuza,

Can you spend some time reviewing this please? I= 've started playing with it as well - the first thing that's irking= me somewhat is the lack of comments. Descriptive function names are all we= ll and good, but sometimes a little more is needed, especially for less exp= erienced developers or newcomers to the application!

=
Sure, already in our list.
On Mo= n, Apr 2, 2018 at 7:45 PM, Joao De Almeida Pereira <jdealmeidap= ereira@pivotal.io> wrote:
<= div dir=3D"ltr">Hi Hackers,

Attached you can find the pa= tch that will start to decouple pgAdmin from ACITree library.
Thi= s 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 mass= ive 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:=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 available test doubles:=C2=A0https://martinfowler.com/bliki/T= estDouble.html) that can be 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 functionalities

0002 patch:=
=C2=A0- Extracts, refactors, adds tests and remove dependency fr= om 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 func= tion
=C2=A0=C2=A0
This patch represents only 10 calls to = ACITree.itemData out of 176 that are spread around our code

<= /div>

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 longe= r 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 functi= on 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 funct= ion that would give us the same information. We had 2 options:
= =C2=A0 a) Create a tree with a function called itemData
Pros:
=C2=A0- At first view this was the simpler solution
=C2=A0- Wou= ld 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 then the node would be responsible for givin= g it's data.
Pros:=C2=A0
=C2=A0- OOP Approach
=
=C2=A0- More flexible and we do not need to bring the tree around, jus= t a node
Cons:
=C2=A0- 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 b= e renamed to ACITreeWrapper and TreeNode.

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

3. We = selected the first file on the search and found the function that was respo= nsible for calling the itemData function.

4. Extra= cted the function to a separate file

5. Wrap this = function with tests

6. Refactor the function to ES= 6, give more declarative names to variables and break the functions into sm= aller 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 th= e original file and use the new function

10. Ensur= e everything still works

11. Find the next functio= n 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. Th= is is just the first step on the direction of completely isolating the ACIT= ree 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 add= ress:
=C2=A0- Finish the complete decoupling of the ACITree
=C2=A0- Performance of the current tree implementation
= =C2=A0- Tweak the naming of the Tree class to explicitly tell us this is to= use only with ACITree.


Thanks
J= oao




--
Dave Page
Blo= g: http://pgsnake= .blogspot.com
Twitter: @pgsnake

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

--0000000000003f72f9056911a2b2--