Skip to content

compression bug fix#46

Merged
jonnypress merged 4 commits intomasterfrom
compression_bug
Apr 3, 2017
Merged

compression bug fix#46
jonnypress merged 4 commits intomasterfrom
compression_bug

Conversation

@mmeenagh1
Copy link
Copy Markdown
Contributor

No description provided.

@mmeenagh1 mmeenagh1 requested a review from jonnypress March 20, 2017 16:30
@mmeenagh1 mmeenagh1 assigned mmeenagh1 and jonnypress and unassigned mmeenagh1 Mar 20, 2017
Comment thread code/processes/wdb.q Outdated

// set .z.zd to control how data gets compressed
setcompression:{[compression] if[ 3= count compression;
.lg.o[`compression;"setting compression level to (",(";" sv string compression),")"];
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you fix up the indentation here. Make it consistent with the rest of the file

Comment thread code/processes/wdb.q
removecompression:{[compression] if[ 3= count compression;
.z.zd:16 0 0;
.lg.o[`compression;"resetting compression level to (",(";" sv string 16 0 0),")"]]}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you make "set compression" and removecompression a single function? (or remove could be a projection of set). So basically if count=3, then set it. if count = 0 (empty list), then unset it

Comment thread code/processes/wdb.q
[.lg.o[`sort;"sorting on slave sort", string .z.p];
{[x] .sort.sorttab[x];if[gc;.gc.run[]]} peach tablist,'.Q.par[dir;pt;] each tablist;
{[x] setcompression[compression];.sort.sorttab[x];removecompression[compression];if[gc;.gc.run[]]} peach tablist,'.Q.par[dir;pt;] each tablist;
];
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't going to work. The sorting here is done on a remote process, which might have different compression parameters defined. You should pass the compression settings to the remote process (as an extra parameter to the function)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh ok, I had been setting values for "compression" in the sort/sortslaves config files. Instead, should the "compression" value set in the wdb config file get used in all 3 cases ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes - it should just be set in one place and it should drive the other processes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've fixed that problem now, but I've had to add a compression parameter into a few functions (informsortandreload, endofdaysort, endofdaymerge, endofdaysortdate) so that it feeds through to the sort/sortslave processes. Is this ok or is there a better way to do it ?

Comment thread code/processes/wdb.q Outdated
merge[dir;pt;;mergelimits] peach flip (key tablist;value tablist);];
removecompression[compression];
[.lg.o[`merge;"merging on master"];
merge[dir;pt;;mergelimits] each flip (key tablist;value tablist)]];
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you need to wrap this block in set/remove statements? This is the bit that does the merging on the master process

Comment thread code/processes/wdb.q Outdated
[.lg.o[`merge;"merging on slave"];
setcompression[compression];
merge[dir;pt;;mergelimits] peach flip (key tablist;value tablist);];
removecompression[compression];
Copy link
Copy Markdown
Contributor

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 will work. setcompression will be executed locally, whereas merge will be executed remotely. You need to wrap the merge statement in a lambda and set/remove compression either side of the merge function call (the same as you do with the other peach statement)

Comment thread code/processes/wdb.q Outdated

endofdaysortdate:{[dir;pt;tablist]
// set .z.zd to control how data gets compressed
setcompression:{[compression] if[3=count compression;.lg.o[`compression;$[compression~16 0 0;"resetting";"setting"]," compression level to (",(";" sv string compression),")"];.z.zd:compression]}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you split this across lines?

you could also add a function of
resetcompression:{setcompression 16 0 0}

which might be nice

@jonnypress jonnypress merged commit 22dbf09 into master Apr 3, 2017
@jonnypress jonnypress deleted the compression_bug branch April 3, 2017 16:00
FlyingOE added a commit to FlyingOE/TorQ that referenced this pull request Apr 6, 2017
FlyingOE added a commit to FlyingOE/TorQ that referenced this pull request May 16, 2017
* tag 'v3.1.0':
  Version num (DataIntellectTech#52)
  Update version.txt (DataIntellectTech#51)
  Add kafka to docs (DataIntellectTech#50)
  kafka for l64 (DataIntellectTech#49)
  compression bug fix (DataIntellectTech#46)
  Datareplay util (DataIntellectTech#48)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants