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

How is your work on this going Ashesh? Will you be committing today?

On Wed, Apr 25, 2018 at 8:52 AM, Dave Page <dpage(at)pgadmin(dot)org> wrote:

> 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(dot)zabuawala(at)enterprisedb(dot)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
>

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

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