Re: [pgadmin4][patch] Initial patch to decouple from ACI Tree

Ashesh; you had agreed to work on this early this week. Please ensure you
do so today.

Thanks.

On Tue, Apr 24, 2018 at 8:13 PM, Joao De Almeida Pereira <
jdealmeidapereira(at)pivotal(dot)io> wrote:

> Hi Hackers,
>
> Can someone review and merge this patch?
>
> Thanks
> Joao
>
> On Wed, Apr 18, 2018 at 10:23 AM Joao De Almeida Pereira <
> jdealmeidapereira(at)pivotal(dot)io> wrote:
>
>> Hi Hackers,
>> Any other comment about this patch?
>>
>> Thanks
>> Joao
>>
>> On Tue, Apr 10, 2018 at 12:00 PM Joao De Almeida Pereira <
>> jdealmeidapereira(at)pivotal(dot)io> wrote:
>>
>>> Hello Khushboo
>>>
>>> On Mon, Apr 9, 2018 at 1:59 AM Khushboo Vashi <
>>> khushboo(dot)vashi(at)enterprisedb(dot)com> wrote:
>>>
>>>> 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(at)pivotal(dot)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.
>>>>
>>>
>>> About the location of the files. The move of the files to
>>> pgadmin/static/js was made on purpose in order to clearly separate
>>> Javascript from python code.
>>> The rational behind it was
>>> - Create a clear separation between the backend and frontend
>>> - Having Javascript code concentrated in a single place, hopefully, will
>>> encourage to developers to look for a functionality, that is already
>>> implemented in another modules, because they are right there. (When we
>>> started this journey we realized that the 'nodes' have a big groups of code
>>> that could be shared, but because the Javascript is spread everywhere it is
>>> much harder to look for it)
>>>
>>>
>>> There are some drawbacks of this separation:
>>> - When creating a new module we will need to put the javascript in a
>>> separate location from the backend code
>>>
>>>
>>>>
>>>>
>>>>> 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 that
>>>>> 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)
>>>>
>>> About the comment point I need a more clear understanding on what kind
>>> of comments you are looking for. Because when you read the function names
>>> you understand the intent, what they are doing. The parameters also explain
>>> what you need to pass into them.
>>>
>>> If what you are looking for in these comments is the reasoning being the
>>> change itself, then that should be present in the commit message. Specially
>>> because this is going to be a very big patch with a very big number of
>>> changes.
>>>
>>>>
>>>> Thanks
>>>>> Joao
>>>>> ​
>>>>>
>>>>> 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 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(at)pivotal(dot)io> wrote:
>>>>>>
>>>>>>> Hi Khushboo,
>>>>>>> Attached you can find both patches rebased
>>>>>>>
>>>>>>> Thanks
>>>>>>>
>>>>>>>
>>>>>>> On Thu, Apr 5, 2018 at 6:31 AM Khushboo Vashi <
>>>>>>> khushboo(dot)vashi(at)enterprisedb(dot)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(at)pivotal(dot)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