-
Notifications
You must be signed in to change notification settings - Fork 17
GPII Orca settings handler #16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Hi Javi, Do you have an example of the updated Solutions JSON and a Preferences JSON for one of the demo users that I can use to try this out? Cheers, |
|
Hi Steve! After this last commit, I think we're almost ready to merge this. Anyway, I'm hoping on include some unit tests in a couple of days. You can use this personal branch 1 of the universal repo to test the Orca settings handler. There you'll find the integration of this settings handler as a solution and a user profile (orca1) for testing purposes. Best regards! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use path.resolve to build paths.
|
Also, @javihernandez, given that it's somewhat similar to the default settings handlers, could you supply some unit tests along with the pull requests. Thanks. |
|
@javihernandez, is this ready for another round of review? I noticed there are still some tweaks missing based on the IRC conversation (http://wiki.fluidproject.org/display/fluid/fluid-work+IRC+Logs-2013-06-20) re common utils method and overall asynchrony.. |
- Using gpii.getJSONFromFileSync - Simplifying some stuff - Code clean up
Removing old gpii.utils.getJSONFromFile, see: * GPII/universal#128 * nodejs/node-v0.x-archive#6073
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get and set parsers are now part of the framework: fluid.model.escapedGetConfig and fluid.model.escapedSetConfig
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just: var curDate;
|
@javihernandez your pull request looks good. Could you also add a TODO note around wait and mention that this needs to be converted to an asynchronous as soon as the lifecycle manager can handle asynchronous actions. Also mention a stability risk since if the file never arrives the system deadlocks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As it stands, this blocking loop is a big stability risk to the system, since it blocks the node event loop possibly indefinitely. If we absolutely must get this in for GPII 0.2 we can accept it, with a huge warning applied to it, but we should move to a properly asynchronous system as soon as possible (requires fixes in universal). Since this code would only be used on a single-user system we could argue that the risk isn't completely serious - but in general we have a lot of work to do in GPII to ensure that all possible sequences of activites, including multiple logon, logoff, and I/O errors always leave the system responsive and in a consistent state.
A simple improvement here might be to impose a maximum waiting time for the file, after which we error out and return control to node - at least we can guarantee then that the system will not block indefinitely
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This IS a must for v0.2 to allow proper support for orca, but a maximum blocking time would probably be a temporary 'solution'.
All the other settingshandlers are also synchronous atm (though with less obvious risks of inifitely blocking the thread) and it seems fixing this requires some rather significant changes to universal - including events fired by each settingshandler and the lifecycle manager listening to them. The integration testing is currently also expecting things to be synchronous, so this would have to be modified as well. imo can put it in, with the suggested max-wait time hack, and a big warning/todo, and then make it a priority to make the handlers asynchronous asap post v0.2 (prob. v0.3 or v0.4).
[List of changes]
* Replacing setParser with fluid.model.escapedSetConfig
* Using just "var curDate"
* Adding an explanation and a warning regarding the "while loop"
blocking issue and its risks
* Adding a max wait time to not block the main loop and return an
error if required
|
Done! Note that I have tested this using the branch from GPII/universal#130 After that, I can log in and out using the token "orca1". Regarding GPII/universal#130, it contains the transformation stuff for this settings handler, and should be updated properly. |
|
BTW, I hope this link helps this pull to be reviewed ;) |
see http://issues.gpii.net/browse/GPII-82
reviewing, testing and commenting will be appreciated ;)
Cheers!