-
Notifications
You must be signed in to change notification settings - Fork 300
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
HPCC-22627 Add skew info to logical files when published #12856
HPCC-22627 Add skew info to logical files when published #12856
Conversation
https://track.hpccsystems.com/browse/HPCC-22627 |
@wangkx - please review. Do dfu_file.xslt (there's 2 of them) need changing as well? |
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.
@jakesmith Please check my 2 questions.
In ws_dfuService.cpp, you may also define out the getFilePartsOnClusters() because it is not needed anymore.
The dfu_file.xslt is for legacy ECLwatch.
} | ||
offset_t avgPartSz = totalPartSz / np; | ||
|
||
maxSkew = (unsigned)(10000.0 * (((double)maxPartSz-avgPartSz)/avgPartSz)); |
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.
Why do 10000.0 * here and /100 in ws_dfuService.cpp? Why not do 100* here?
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.
To keep it as an unsigned with 2 point precision. When it is divided by 100 it may become e.g. 123.45%
Stat.setMaxSkewPart(maxSkewPart); | ||
} | ||
|
||
VStringBuffer minSkewString("-%.2f", ((double)minSkew)/100); |
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.
Should the minus be added if minSkew = 0?
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.
@jakesmith did you miss this comment?
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 did miss it, thanks. I agree should be removed.
I'll change.
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.
@wangkx - have now amended to avoid adding minus if no skew.
@wangkx - should they be kept updated? |
I did not mean to leave defined out, I meant to delete, but thanks, yes I'll also delete getFilePartsOnClusters. |
If you can update the dfu_file.xslt, it will be great. We do not support the legacy ECLWatch officially. But, someone still uses it. I haven't found any code being removed is needed. |
63e6f0b
to
9d39ed9
Compare
@wangkx - have deleted the defined out code and getFilePartsOnClusters() Please review. |
9d39ed9
to
d4a8a6e
Compare
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.
@jakesmith looks fine.
@richardkchapman - please merge. |
@jakesmith Should this target 7.6? |
Expose on file details page. Signed-off-by: Jake Smith <jake.smith@lexisnexisrisk.com>
d4a8a6e
to
e9a2e17
Compare
@richardkchapman - yeah think it should, have rebased |
Automated Smoketest: ✅ Unit tests result:
Regression test result:
HPCC Stop: OK
|
The code was removed by: hpcc-systems#12856. The code is needed for ECLWatch to retrieve file parts information. Signed-off-by: wangkx <kevin.wang@lexisnexis.com>
Expose on file details page.
Signed-off-by: Jake Smith jake.smith@lexisnexisrisk.com
Type of change:
Checklist:
Smoketest:
Testing: