Adding mutation data to CAMEL database #39
Conversation
Change Mutation data field (36) from value_ATTACH to value_BOOL
Added mutation tables
Doesn't work? Reroutes /home???
Still need to fix API to display correct data
Need to clean data
Joining & Subquerying in MariaDB is terrible
|
Hi @BramVDBe , |
|
Hi @MaybeJustJames, thanks for getting back at this. As for why @nichael-meilson shifted from MariaDB to MySQL, I'll leave that to him to explain to avoid making wrong assumptions. |
|
I switched from MariaDB to MySQL because joining and subquerying larger tables in MariaDB was slow enough to timeout GET requests from the browser. There's a paper on it here regarding join performance. I chose MySQL because it's mostly compatible with MariaDB as a "drop-in replacement". I didn't really know how to go about scaling the database to handle mutation data without switching away from MariaDB - do you know if there's another way? |
|
Hi @nichael-meilson, @BramVDBe, @nichael-meilson I do remember you had performance issues. In fact we discussed using simulated materialised views as a potential solution:
What came of this? I'm pushing back hard on this because switching database server on the R910 is, to dramatically understate it, not a trivial task. For perspective, that's true wherever you host CAMEL: switching SQL implementations even ones as compatible as Maria and MySQL is not a decision to be taken lightly. |
There was a problem hiding this comment.
Ok @nichael-meilson, these comments are a brief look over your work. It's clear you've put a lot of effort into this and I'm confident these changes work for you. My job here is to ensure that we can continue to maintain them and ensure we can safely deploy them.
To this end there are a couple of major things I'd like you to consider doing for me please (in order of priority):
- Use a code formatter on the js: my suggestion
- Add some unit tests. This isn't going to be easy since there are no existing tests and your code wasn't written with testing in mind. The trade-off is that it they make me more confident that I can safely accept your changes. Ideally I'd like you to test only your changes using frameworks like Py.Test for the server, and Jest for the client.
- Do some linting. ESLint which integrates with Prettier for the client, and PyLint for the server.
The above requests only apply to your changes. Please don't feel you have to change any of the existing code. Please don't change existing code (in this PR; you're extremely welcome to change existing code in a different PR).
These comments seem harsh. I don't mean to belittle the huge effort you've put in to get as far as you have. Really good job. I hope you can put some more effort in so we can deploy your work :)
public/app/app.resources.js
Outdated
| update: {method: 'PUT'} | ||
| }); | ||
| }) | ||
| .factory("Mutation", function MutationFactory($resource, config) { |
There was a problem hiding this comment.
Be careful with consistent formatting. You're using TAB characters in a space indented file.
public/app/app.resources.js
Outdated
| return $resource(config.apiUrl+"/mutation/:id", {id: '@id'}, { | ||
| update: {method: 'PUT'} | ||
| }); | ||
| }) |
public/app/app.resources.js
Outdated
| update: {method: 'PUT'} | ||
| }); | ||
| }) | ||
| .factory("FieldMut", function FieldMutFactory($resource, config) { |
public/app/app.resources.js
Outdated
| return $resource(config.apiUrl+"/field_mut/:id", {id: '@id'}, { | ||
| update: {method: 'PUT'} | ||
| }); | ||
| }) |
public/app/app.routes.js
Outdated
| controller: 'ExperimentsController', | ||
| controllerAs: 'experiments' | ||
| }) | ||
| .when('/mutation/:id', { |
| if (State.expFields.length == 0){ | ||
| ctrl.fields = FieldMut.query(function(){ | ||
| var i = 0; | ||
| for (var field_i in ctrl.fields){ |
There was a problem hiding this comment.
What is this supposed to do?
| ctrl.toggleFilterItem = function(field){ | ||
| if (field.filter){ | ||
| field.filter = false; | ||
| if (field.hasOwnProperty('type_column')){ |
There was a problem hiding this comment.
What is all this logic supposed to do?
| field.show = !field.show; | ||
| } | ||
|
|
||
| ctrl.filterUpdate = function(filter_id){ |
| scope.itemsPerPage = 20; | ||
|
|
||
| //Sort functionality | ||
| if (!State.expOrder.hasOwnProperty('key')) { |
There was a problem hiding this comment.
You're not using State.expOrder.key, why are you checking for it?
| scope.orderParams = State.expOrder; | ||
| } | ||
|
|
||
| scope.sortExperiments = function (key, keyRealm = 'exp') { |
There was a problem hiding this comment.
I really prefer you not use default parameter values, please be explicit
|
@nichael-meilson if you're at all confused about how to work on my suggestions please don't try to guess your way through. Let me know and we can pair on it. |
|
Ping @nichael-meilson ? |
|
Hey @MaybeJustJames, I'm a bit busy with work (+ I have to re-sit some exams) so I was planning on going over your comments on September 4th. Do you have a deadline in mind for this? |
No worries from our side @nichael-meilson . You should talk to @BramVDBe about a deadline. Best of luck with your exams :) |
|
Hi @nichael-meilson, any update on this? kind regards |
|
Hi @BramVDBe, Progress has been slow as I was setting up a development environment on a new computer but things seem to be working now. I'm nervous to see how buggy my code is when I start testing but it's all necessary |
Don't be nervous about this @nichael-meilson . You are not your code and finding bugs now means users won't see them in prod. By the way, can you respond to my MySQL materialized views query above? I want to know why triggering updates to a cache table (simulated materialized views) wasn't an option? |
|
Ping @nichael-meilson , any progress? Please let me know if you're struggling |
|
Ping @nichael-meilson |
|
Hi All, sorry for the delayed reply - I've been swamped with work lately. Fortunately I think I've figured out how to appropriately test the API, but unfortunately, there needs to be some massive overhauling to the API. For instance, making a
And convert them to a format like this to match the DB schema which is made with JavaScript functionality in mind, like this:
Which, to put lightly, is a pain, and to put heavily, slows down data retrieval performance exponentially Regarding MariaDB, it doesn't support nested querying (current solution) or materialized views. Apparently it only supports (non-materialized) views which I don't think is the solution to this issue. Additionally, I'm not extremely versed in more complex concepts of SQL so I would have to learn how to do all of this before determining if it's an option As for the JavaScript testing, I don't actually know how to write JavaScript and basically learned enough of the angular concepts to decipher existing UI and replicate it so I genuinely don't know how/where one would begin to test So yeah, it's still a ton of work from my perspective and I'm even having a hard time figuring out how many things need to be fixed and in what order they need to be done |
|
Hi @nichael-meilson, Is there anywhere @MaybeJustJames could help out to make it a more efficient transition? I know James is also swamped likely but perhaps there things that he could perform swiftly? |
I wasn't sure how to effectively make this PR smaller because so many of the commits are intertwined.
In essence, very few of the existing components of CAMEL were altered (only a switch from MariaDB to MySQL which doesn't seem to have any major effect). Mutation data functionality was added to the CAMEL API, SQL database, and a data querying interface - similar to the "experiments" - for the front end