MIME-Version: 1.0
In-Reply-To: 
 <CAE+jja=Gdd032H7tpoZD2C0m2R7SnTZpHX_oPx2K2zGbaaW9yg@mail.gmail.com>
References: 
 <CAE+jja=Gdd032H7tpoZD2C0m2R7SnTZpHX_oPx2K2zGbaaW9yg@mail.gmail.com>
From: Khushboo Vashi <khushboo.vashi@enterprisedb.com>
Date: Thu, 5 Apr 2018 16:01:48 +0530
Message-ID: 
 <CAFOhELcrZsRkwV4beOC7UT_HYvi9Rk=HFJ355g8Bj4bqyzMLog@mail.gmail.com>
Subject: Re: [pgadmin4][patch] Initial patch to decouple from ACI Tree
To: Joao De Almeida Pereira <jdealmeidapereira@pivotal.io>
Cc: pgadmin-hackers <pgadmin-hackers@postgresql.org>
Content-Type: multipart/alternative; boundary="0000000000006ced310569177209"
Precedence: bulk

--0000000000006ced310569177209
Content-Type: text/plain; charset="UTF-8"

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
>
>

--0000000000006ced310569177209
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr">Hi Joao,<div><br></div><div>Can you please rebase the seco=
nd patch?</div><div><br></div><div>Thanks,</div><div>Khushboo<br><div class=
=3D"gmail_extra"><br><div class=3D"gmail_quote">On Tue, Apr 3, 2018 at 12:1=
5 AM, Joao De Almeida Pereira <span dir=3D"ltr">&lt;<a href=3D"mailto:jdeal=
meidapereira@pivotal.io" target=3D"_blank">jdealmeidapereira@pivotal.io</a>=
&gt;</span> wrote:<br><blockquote class=3D"gmail_quote" style=3D"margin:0 0=
 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir=3D"ltr">Hi Ha=
ckers,<div><br></div><div>Attached you can find the patch that will start t=
o decouple pgAdmin from ACITree library.</div><div>This patch is intended t=
o 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.</div>=
<div><br></div><div>This job that we started is a massive tech debt chore t=
hat will take some time to finalize and we would love the help of the commu=
nity to do it.</div><div><br></div><div><b>Summary of the patch:</b></div><=
div>0001 patch:=C2=A0</div><div>=C2=A0- Creates a new tree that will allow =
us to create a separation between the application and ACI Tree</div><div>=
=C2=A0- Creates a Fake Tree (Test double, for reference on the available te=
st doubles:=C2=A0<a href=3D"https://martinfowler.com/bliki/TestDouble.html"=
 target=3D"_blank">https://martinfowler.<wbr>com/bliki/TestDouble.html</a>)=
 that can be used to inplace to replace the ACITree and also encapsulate th=
e new tree behavior on our tests</div><div>=C2=A0- Adds tests for all the t=
ree functionalities</div><div><br></div><div>0002 patch:</div><div>=C2=A0- =
Extracts, refactors, adds tests and remove dependency from ACI Tree on:</di=
v>   - getTreeNodeHierarchy <br>    - on backup.js: menu_enabled, menu_enab=
led_server, start_backup_global_server, backup_objects<br>    - on datagrid=
.js: show_data_grid, get_panel_title, show_query_tool<div>- Start using spr=
intf-js as Underscore.String is deprecating sprintf function<br><div>=C2=A0=
=C2=A0</div><div>This patch represents only 10 calls to ACITree.itemData ou=
t of 176 that are spread around our code</div><div><br></div><div><b><br></=
b></div><div><b>In Depth look on the process behind the patch:</b></div><di=
v><br></div><div>We started writing this patch with the idea that we need t=
o decouple pgAdmin4 from ACITree, because ACITree is no longer supported, t=
he documentation is non existent and ACITree is no longer being actively de=
veloped.</div><div><br></div><div>Our process:</div><div>1. We &quot;random=
ly&quot; selected a function that is part of the ACITree. From this point w=
e decided to replace that function with our own version. The function that =
we choose was &quot;itemData&quot;.</div><div>The function gives us all the=
 &quot;data&quot; that a specific node of the tree find.</div><div>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:</div><div>=C2=A0 a) Create a tr=
ee with a function called itemData</div><div>Pros:</div><div>=C2=A0- At fir=
st view this was the simpler solution</div>=C2=A0- Would keep the status qu=
o<div>Cons:</div><div>=C2=A0- Not a OOP approach</div><div>=C2=A0- Not very=
 flexible</div><div>=C2=A0 b) Create a tree that would return a node given =
an ID and then the node would be responsible for giving it&#39;s data.</div=
><div>Pros:=C2=A0</div><div>=C2=A0- OOP Approach</div><div>=C2=A0- More fle=
xible and we do not need to bring the tree around, just a node</div><div>Co=
ns:</div><div>=C2=A0- Break the current status quo</div><div><br></div><div=
>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 ACITreeW=
rapper and TreeNode.</div><div><br></div><div>2. After we decided on the st=
arting point we searched for occurrences of the function &quot;itemData&quo=
t; and we found out that there were 303 occurrences of &quot;itemData&quot;=
 in the code and roughly 176 calls to the function itself (some of the hits=
 were variable names).</div><div><br></div><div>3. We selected the first fi=
le on the search and found the function that was responsible for calling th=
e itemData function.</div><div><br></div><div>4. Extracted the function to =
a separate file</div><div><br></div><div>5. Wrap this function with tests</=
div><div><br></div><div>6. Refactor the function to ES6, give more declarat=
ive names to variables and break the functions into smaller chunks</div><di=
v><br></div><div>7. When all the tests were passing we replaced ACITree wit=
h our Tree</div><div><br></div><div>8. We ensured that all tests were passi=
ng</div><div><br></div><div>9. Remove function from the original file and u=
se the new function</div><div><br></div><div>10. Ensure everything still wo=
rks</div><div><br></div><div>11. Find the next function and execute from st=
ep 4 until all the functions are replaced, refactored and tested.</div><div=
><br></div><div>As you can see by the process this is a pretty huge underta=
ke, 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 so=
lve the problem with a large number of elements on the tree.</div><div><br>=
</div><div><b>What is on our radar that we need to address:</b></div><div>=
=C2=A0- Finish the complete decoupling of the ACITree</div><div>=C2=A0- Per=
formance of the current tree implementation</div><div>=C2=A0- Tweak the nam=
ing of the Tree class to explicitly tell us this is to use only with ACITre=
e.</div><div><br></div><div><br></div><div>Thanks</div><span class=3D"HOEnZ=
b"><font color=3D"#888888"><div>Joao</div><div><br></div></font></span></di=
v></div>
</blockquote></div><br></div></div></div>

--0000000000006ced310569177209--