-
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-542 DFSClient: Create JUnit for read retry #648
Conversation
- Added file part failure retry test - Fixed retry issue on initial connection Signed-off-by: James McMullan [email protected]
@rpastrana squashed and pushed up code review changes. The first block of the test is the original test that had resulted in the crash |
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.
@jpmcmu a few comments
*/ | ||
public void setCopyIP(int copyIndex, String copyIP) | ||
{ | ||
if (copyIndex < 0 || copyIndex >= copyLocations.length) return; |
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.
let's move the return statement to the next line, for optics and future maintenance, but more importantly, do we need to throw in this situation?
{ | ||
List<String> copyLocationsList = new ArrayList<>(Arrays.asList(copyLocations)); | ||
copyLocationsList.add(index, copyIP); | ||
copyLocations = copyLocationsList.toArray(new String[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.
this seems a little bit extraneous, not sure what type copyLocations is, but are we not able to append the copyIP directly? and if not, are we using the most appropriate datastructure to track copyIPs, copypaths?
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.
Agreed, the existing type is a simple array; I had considered moving this to a List but other parts of the code are expecting an array, so it would require the internal list be converted to an Array at those locations. I thought it would be better to do the conversion here in the add() function because this unlikely to be used outside of test cases.
*/ | ||
public void add(int index, String copyIP, String copyPath) | ||
{ | ||
List<String> copyLocationsList = new ArrayList<>(Arrays.asList(copyLocations)); |
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 either index, copyIP, or copyPath are invalid, we should throw
try | ||
{ | ||
if (getUseSSL()) | ||
log.debug("Attempting to connect to file part : '" + dataPart.getThisPart() + "' Copy: '" | ||
+ (getFilePartCopy() + 1) + "' on IP: '" + getIP() + "'"); |
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.
worth reporting the filename and filepart path 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.
Good question, we don't have access to the filename at this point, but I can log the file path.
// So we don't care as much about individual packet latency or connection time overhead | ||
sock.setPerformancePreferences(0, 1, 2); | ||
sock.connect(new InetSocketAddress(this.getIP(), this.dataPart.getPort()), this.connectTimeout); | ||
throw new HpccFileException("Bad file part addr " + this.getIP(), e); |
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.
could this be due to invalid path 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.
@jpmcmu approved
Signed-off-by: James McMullan [email protected]
Type of change:
Checklist:
Testing: