-
Notifications
You must be signed in to change notification settings - Fork 82
Idb dataaccess integ #702
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
base: master
Are you sure you want to change the base?
Idb dataaccess integ #702
Conversation
code/processes/idb.q
Outdated
| /-send sync message to WDB to register the existing IDBs. | ||
| @[w;(`.servers.registerfromdiscovery;`idb;0b);{.lg.e[`connection;"Failed to register IDB with WDB."];'x}]; | ||
| /-dataaccess initialisation must be done after wdb loaded. | ||
| if[&[`dataaccess in key .proc.params;.proc.proctype=`idb];.dataaccess.init[]]; |
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.
@jonathonmcmurray as discussed last Fri probably a cleaner way to do this...
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.
I don't think you need to check the proctype? if loading this file, surely it's an idb, regardless of proctype?
| if[&[`dataaccess in key .proc.params;.proc.proctype=`idb];.dataaccess.init[]]; | |
| if[`dataaccess in key .proc.params;.dataaccess.init[]]; |
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.
Yes of course!
code/gateway/dataaccess.q
Outdated
| // Let idb take precedence over rdb to prevent data duplication | ||
| if[all `rdb`idb in key procdict;procdict:delete rdb from procdict]; |
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.
I don't think this is something we want to do by default in TorQ - it may be appropriate in some individual TorQ-based systems, but in general, I don't think we want to exclude all RDB data just because there is an IDB. A typical pattern we expect people to use would be having e.g. most recent hour in RDB and everything up until last write in IDB. If we only return data from IDB, we'll be missing anything received since last write, even though that data would be available in RDB.
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.
Removed this hardcoded removal of rdb if idb is present. As discussed if client has idb/rdb up with full day's worth of data and queries both they will get duplication but that is the correct thing to do here. They can easily dedeupe by using proc arg in input dict
code/gateway/dataaccess.q
Outdated
| options:@[@[options;`starttime;:;"p"$options`starttime];`endtime;:;$[-14h~type et:options`endtime;-1+1D+et;"p"$et]]; //ensure st/et are timestamps; if date adjust endtime | ||
| partitions:{x[0],x[1]+1D-1}each partitions; //create dict of datetime coverage for each process | ||
| partitions:@[partitions;f;:;(st:"p"$options`starttime;min(et:"p"$options`endtime;partitions[f:first key partitions;1]))]; //amend query datetimes on hdb | ||
| partitions:@[partitions;l;:;(max(st;partitions[l:last key partitions;0]);et)]; //amend query datetimes on rdb/idb |
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 file uses comments above lines of code, please keep that consistent
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.
Done
code/gateway/dataaccess.q
Outdated
| // convert first and last timestamp to start and end time | ||
| partitions:@[partitions;f;:;(start;partitions[f:first key partitions;1])]; | ||
| partitions:@[partitions;l;:;(partitions[l:last key partitions;0];options`endtime)]]; | ||
| // adjust the query times accordingly |
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 comment is very vague - what does "accordingly" mean in this context?
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.
Removed
code/processes/idb.q
Outdated
| /-send sync message to WDB to register the existing IDBs. | ||
| @[w;(`.servers.registerfromdiscovery;`idb;0b);{.lg.e[`connection;"Failed to register IDB with WDB."];'x}]; | ||
| /-dataaccess initialisation must be done after wdb loaded. | ||
| if[&[`dataaccess in key .proc.params;.proc.proctype=`idb];.dataaccess.init[]]; |
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.
I don't think you need to check the proctype? if loading this file, surely it's an idb, regardless of proctype?
| if[&[`dataaccess in key .proc.params;.proc.proctype=`idb];.dataaccess.init[]]; | |
| if[`dataaccess in key .proc.params;.dataaccess.init[]]; |
No description provided.