On Mon, Apr 30, 2018 at 8:58 PM, Anthony Emengo <aeme...@pivotal.io> wrote:
> I was expecting a separate layer between the tree implementation, and >> aciTree adaptor. >> Please find the patch for the example. >> It will separate the two layers, and easy to replace with the new >> implementation in future. > > > In general, we want defer the separation of the layers for now. Even > though we might assume that this is the direction we want to go in. It's > simply too early to be making such an architectural leap. For right now, we > just know that we need the decoupling, but don't know what if we'd need the > 2 layers *as implemented*. The principle we're adhering to here is the > Last Responsible Moment principle, which states that you only make the > changes that you feel is necessary for the given problems you're facing: > https://blog.codinghorror.com/the-last-responsible-moment/ > > I would not like to see that changes in this patch. >> I would like us to come up with the actual design about the hot >> pluggability before going in this direction. > > > In our point of view these 2 changes are not related. One thing is the > internal code organization of the application, other thing is allowing > third party to drop code in the application and it just work. These 2 > should be talked separately, but the hot pluggability is not something that > will be address by this work we are doing right now. > Neither - it should be part of this change. It should be addressed separately, and discussed. -- Thanks, Ashesh > > Anthony && Joao > > On Mon, Apr 30, 2018 at 11:03 AM, Ashesh Vashi < > ashesh.va...@enterprisedb.com> wrote: > >> >> >> >> On Mon, Apr 30, 2018 at 8:30 PM, Ashesh Vashi < >> ashesh.va...@enterprisedb.com> wrote: >> >>> On Sat, Apr 28, 2018 at 3:55 AM, Joao De Almeida Pereira < >>> jdealmeidapere...@pivotal.io> wrote: >>> >>>> Hi Hackers, >>>> As you are aware we kept on working on the patch, so we are attaching >>>> to this email a new version of the patch. >>>> This new version contains all the changes in the previous one plus more >>>> extractions of functions and refactoring of code. >>>> >>>> The objective of this patch is to create a separation between pgAdmin >>>> and the ACI Tree. We are doing this because we realized that at this point >>>> in time we have the ACI Tree all over the code of pgAdmin. I found a very >>>> interesting article that really talks about this: >>>> https://medium.freecodecamp.org/code-dependencies-are-the-de >>>> vil-35ed28b556d >>>> >>>> In this patch there are some visions and ideas about the location of >>>> the code, the way to organize it and also try to pave the future for a >>>> application that is stable, easy to develop on and that can be release at a >>>> times notice. >>>> >>>> We are investing a big chunk of our time in doing this refactoring, but >>>> while doing that we also try to respond to the patches sent to the mailing >>>> list. We would like the feedback from the community because we believe this >>>> is a changing point for the application. The idea is to change the way we >>>> develop this application, instead of only correcting a bug of developing a >>>> feature, with every commit we should correct the bug or develop a feature >>>> but leave the code a little better than we found it (Refactoring, >>>> refactoring, refactoring). This is hard work but that is what the users >>>> from pgAdmin expect from this community of developers. >>>> >>>> >>>> ====== >>>> >>>> >>>> >>>> It is a huge patch >>>> 86 files changed, 5492 inserts, 1840 deletions >>>> and we would like to get your feedback as soon as possible, because we >>>> are continuing to work on it which means it is going to grow in size. >>>> >>>> >>>> At this point in time we still have 124 of 176 calls to the function >>>> itemData from ACITree. >>>> >>>> What does each patch contain: >>>> 0001: >>>> Very simple patch, we found out that the linter was not looking into >>>> all the javascript test files, so this patch will ensure it is >>>> >>> Committed the patch along with the regression introduced because of this >>> patch. >>> >>>> >>>> 0002: >>>> New Tree abstraction. This patch contains the new Tree that works as >>>> an adaptor for ACI Tree and is going to be used on all the extractions that >>>> we are doing. >>>> >>> >>> I was expecting a separate layer between the tree implementation, and >>> aciTree adaptor. >>> Please find the patch for the example. >>> >>> It will separate the two layers, and easy to replace with the new >>> implemenation in future. >>> >> >> Oops forgot to attach the patch. >> Please find the patch attached. >> >> -- Thanks, Ashesh >> >>> >>>> 0003: >>>> Code that extracts, wrap with tests and replace ACI Tree invocations. >>>> >>> There are many small cases left in the patches. >>> Hence - I would like to know the TODO list created by you. >>> >>> e.g. When we remove any of the object from the database server, we're >>> not yet removing the respective node from the new implementation, and its >>> children. >>> >>>> >>>> We start creating new pattern for the location of Javascript files >>>> and their structure. >>>> >>> I would not like to see that changes in this patch. >>> I would like us to come up with the actual design about the hot >>> pluggability before going in this direction. >>> >>>> Create patterns for creation of dialogs (backup and restore) >>>> >>> It's better - we don't change the directory structure at the moment. >>> >>> I am not against dividing the big javascript files in small chunks, but >>> - I would like us to discuss first about the hot plugins design first. >>> >>> -- Thanks, Ashesh >>> >>>> >>>> >>> >>>> >>>> Thanks >>>> Joao >>>> >>>> >>>> On Fri, Apr 27, 2018 at 5:34 AM Ashesh Vashi < >>>> ashesh.va...@enterprisedb.com> wrote: >>>> >>>>> I have quite a few comments for the patch. >>>>> I will send them soon. >>>>> >>>>> On Fri, Apr 27, 2018, 14:45 Dave Page <dp...@pgadmin.org> wrote: >>>>> >>>>>> How is your work on this going Ashesh? Will you be committing today? >>>>>> >>>>>> On Wed, Apr 25, 2018 at 8:52 AM, Dave Page <dp...@pgadmin.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 < >>>>>>> jdealmeidapere...@pivotal.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 < >>>>>>>> jdealmeidapere...@pivotal.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 < >>>>>>>>> jdealmeidapere...@pivotal.io> wrote: >>>>>>>>> >>>>>>>>>> Hello Khushboo >>>>>>>>>> >>>>>>>>>> On Mon, Apr 9, 2018 at 1:59 AM Khushboo Vashi < >>>>>>>>>> khushboo.va...@enterprisedb.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 < >>>>>>>>>>> jdealmeidapere...@pivotal.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.zabuaw...@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 < >>>>>>>>>>>>> jdealmeidapere...@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.va...@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 < >>>>>>>>>>>>>>> jdealmeidapere...@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 >>>>>>> >>>>>> >>>>>> >>>>>> >>>>>> -- >>>>>> Dave Page >>>>>> Blog: http://pgsnake.blogspot.com >>>>>> Twitter: @pgsnake >>>>>> >>>>>> EnterpriseDB UK: http://www.enterprisedb.com >>>>>> The Enterprise PostgreSQL Company >>>>>> >>>>> >>> >> >
