-
Notifications
You must be signed in to change notification settings - Fork 24
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
HPCC4J-595 Enhance log messages #709
Conversation
Jira Issue: https://hpccsystems.atlassian.net/browse/JAPI-595 Jirabot Action Result: |
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.
@drealeed Thanks for the PR! Those are definitely some good changes to make, a few comments / changes.
* @throws Exception | ||
* general exception | ||
*/ | ||
public HpccRemoteFileReader(DataPartition dp, FieldDef originalRD, IRecordBuilder recBuilder, int connectTimeout, int limit, boolean createPrefetchThread, int readSizeKB, FileReadResumeInfo resumeInfo, int socketOpTimeoutMs, String fileName) throws Exception |
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.
Passing in the fileName here is a great idea to improve the logging output, and it probably should always have been available within DataPartition. Would you be willing to move fileName into DataPartition? Ideally we would then pass in the filename to DataPartition.createPartitions from HPCCFile.
{ | ||
this.recordDefinition = rd; | ||
this.fileName =fileName; |
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.
If you implement the above change to DataPartition we could then grab the fileName from there.
private static final String[] datasets = { "~benchmark::integer::20kb", "~unit_test::all_types::thor", "~unit_test::all_types::xml", "~unit_test::all_types::json", "~unit_test::all_types::csv" }; | ||
private static final int[] expectedCounts = { 1250, 10000, 10000, 10000, 10000, 10000}; | ||
private static final String[] datasets = { "~benchmark::integer::20kb", "~benchmark::all_types::200kb"};//, "~unit_test::all_types::xml", "~unit_test::all_types::json", "~unit_test::all_types::csv" }; | ||
private static final int[] expectedCounts = { 1250, 5600 };//, 10000, 10000, 10000, 10000}; |
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.
Lets revert these changes, I know there are test failures here with the text files, but those are being addressed.
@@ -57,10 +57,10 @@ public abstract class BaseRemoteTest | |||
|
|||
protected final static String connString = System.getProperty("hpccconn", "http://localhost:8010"); |
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.
Lets revert these changes as well
@drealeed Looks good, thanks for implementing those changes. If you can squash, I will merge the PR in. |
Enhance error logging to help pinpoint future errors more specifically
Type of change:
Checklist:
Testing: