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

Transferring albumless photos #585

Merged
merged 6 commits into from Oct 1, 2018

Conversation

Projects
None yet
2 participants
@olsona
Contributor

olsona commented Sep 21, 2018

Basics of transferring albumless photos.

Due to API limitations, this is a somewhat inefficient process. First we make a list of all contained (i.e., belonging to an album) photos that a user has by first listing all the containers and then the contents of those containers, and we store the list of contained items in the job store. We then split the transfer into two branches - one for albums and all of their contents, and one for albumless items. The album branch behaves as expected. The albumless branch lists all items in a user's library, and only transfers those items that don't appear on the list of contained items. This way, we avoid importing items twice.

#581

@olsona olsona requested review from bwillard and holachuy Sep 21, 2018

@@ -188,14 +191,92 @@ public GooglePhotosExporter(GoogleCredentialFactory credentialFactory) {
return new ExportResult<>(resultType, containerResource, continuationData);
}
private List<PhotoModel> getAlbumlessPhotos(TokensAndUrlAuthData authData) throws IOException {

This comment has been minimized.

@bwillard

bwillard Sep 24, 2018

Contributor

Instead of doing it this way, which as you indicate requires storing all of the photos in memory at once, why not do something like:

Page through all the albums, and then photos in the album (kind of like what are are doing) and then store each of those photo IDs in the jobstore. Then you don't have to store them all in memory at once.

The other benefit, which is more significant, is that then we don't need to pass all the albumless photos back in one page of results, but instead can just page through things normally.

olsona added some commits Sep 24, 2018

@@ -73,30 +82,34 @@ public GooglePhotosExporter(GoogleCredentialFactory credentialFactory, JsonFacto
public ExportResult<PhotosContainerResource> export(UUID jobId, TokensAndUrlAuthData authData,
Optional<ExportInformation> exportInformation) throws IOException {
if (!exportInformation.isPresent()) {
populateContainedPhotosList(jobId, authData);

This comment has been minimized.

@bwillard

bwillard Oct 1, 2018

Contributor

Maybe add a comment about why we are doing this?

This comment has been minimized.

@olsona

olsona Oct 1, 2018

Contributor

Done.

boolean containerResourcePresent = idOnlyContainerResource != null;
boolean paginationDataPresent = paginationToken != null;
if (!containerResourcePresent

This comment has been minimized.

@bwillard

bwillard Oct 1, 2018

Contributor

Want to add some comments about the expected flow here?

This comment has been minimized.

@olsona

olsona Oct 1, 2018

Contributor

I believe this clarifies some things.

throws IOException {
// Get list of all album ids
List<String> albumIds = new LinkedList<>();
AlbumListResponse albumListResponse = getOrCreatePhotosInterface(authData)

This comment has been minimized.

@bwillard

bwillard Oct 1, 2018

Contributor

I think if you swapped this to a do {}while() you could skip duplicating some code.

This comment has been minimized.

@olsona

olsona Oct 1, 2018

Contributor

Much cleaner, thank you.

}
// Get list of all ids belonging to photos belonging to albums
TempPhotosData tempPhotosData = jobStore

This comment has been minimized.

@bwillard

bwillard Oct 1, 2018

Contributor

This seems confusing to me, why create it once and then update it bellow?

Isn't the logic that we just write this data once? Why not construct the data to store and then just insert it?

This comment has been minimized.

@olsona

olsona Oct 1, 2018

Contributor

I think in general we want to pull the temp data and add to it, but since there won't be any temp data to start with for this case, it should be fine.

jobStore.create(jobId, createCacheKey(), tempPhotosData);
}
for (String albumId : albumIds) {
MediaItemSearchResponse albumMediaItemResponse = getOrCreatePhotosInterface(authData)

This comment has been minimized.

@bwillard

bwillard Oct 1, 2018

Contributor

Is there a reason to re-crete the photos interface every time? If so we should document it.

This comment has been minimized.

@olsona

olsona Oct 1, 2018

Contributor

I believe that pattern is to allow for the possibility that auth data might expire. We haven't written any code to address that yet, however.

.collect(Collectors.toList()));
}
}
}

This comment has been minimized.

@bwillard

bwillard Oct 1, 2018

Contributor

I think a do {} while() will be cleaner here too.

void populateContainedPhotosList(UUID jobId, TokensAndUrlAuthData authData)
throws IOException {
// Get list of all album ids
List<String> albumIds = new LinkedList<>();

This comment has been minimized.

@bwillard

bwillard Oct 1, 2018

Contributor

Any reason to have albumIds as a separate variable, this seems like just two nested loops is a little cleaner right?

jobStore.update(jobId, createCacheKey(), tempPhotosData);
}
private Optional<String> getPaginationToken(Optional<PaginationData> paginationData) {

This comment has been minimized.

@bwillard

bwillard Oct 1, 2018

Contributor

Maybe rename this to getPhotosPagnationToken? right now I would think it would work with albums too.

This comment has been minimized.

@olsona

olsona Oct 1, 2018

Contributor

Done.

for (GoogleMediaItem mediaItem : containedMediaSearchResponse.getMediaItems()) {
tempPhotosData.addContainedPhotoId(mediaItem.getId());
}
} while ((photoToken = containedMediaSearchResponse.getNextPageToken()) != null);

This comment has been minimized.

@bwillard

bwillard Oct 1, 2018

Contributor

I would avoid assignments in conditionals, they get confusing.

This comment has been minimized.

@olsona

olsona Oct 1, 2018

Contributor

Done.

}
}
jobStore.update(jobId, createCacheKey(), tempPhotosData);
} while ((albumToken = albumListResponse.getNextPageToken()) != null);

This comment has been minimized.

@bwillard

bwillard Oct 1, 2018

Contributor

Here too

@olsona olsona merged commit 736c2ab into master Oct 1, 2018

3 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@olsona olsona deleted the albumless-photos branch Oct 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment