-
Notifications
You must be signed in to change notification settings - Fork 14
Lazy iterator and other small improvements #325
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
Conversation
…eException artifacts.
…ake it lazy instead of reallocating the same list over and over again.
isTruncated = result.getTruncated(); | ||
marker = result.getNextMarker(); | ||
remainingKeys -= result.getObjects().size(); | ||
return new LazyObjectIterable(client, bucket, keyPrefix, nextMarker, maxKeys, 5); |
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.
5? dont you want this is be called DefaultRetries or something?
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 call
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.
Fixed
private final int maxKeys; | ||
private final int retryCount; | ||
|
||
public LazyObjectIterable(final Ds3Client client, final String bucket, final String keyPrefix, final String nextMarker, final int maxKeys, final int retryCount) { |
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.
OR you can add a new constructor here that calls the other constructor with default retry value, this way this class contains all the behavior
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.
There was already the telescoping constructors in the helper functions, so I didn't want to move it over here 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.
i just don't think that the helper class should hold the default for this calss
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.
The reason I think it should is that the helper function is currently where all that information exists. And rather than duplicating the telescoping constructors over into the iterator, it's all kept in one place.
return; | ||
} catch (final IOException e) { | ||
if (retryAttempt >= retryCount) { | ||
throw new RuntimeException("Failed to get the next set of objects from the getBucket request", 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.
we need to add to this message that we reached the max number of retries
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 catch
No description provided.