This is tricky, since we let failedLocations be on its own in the last PR, as well as BigQuery's insertErrors, I believe, which is a similar concept. However, we are also breaking consistency from all of our other paginated methods, which use err, objects, nextQuery, apiResponse. I think we should find a way to get this down to the usual callback length by either combining appProfiles and failedLocations, or just documenting where the user can find the failed locations in the apiResponse. What do you think?

The inconsistency comes from the fact that next_page_token is deprecated from listClusters/InstancesResponse, which makes nextQuery obsolete for those functions. However next_page_token is supported in all other paginated responses.

I think, for consistency purposes, we can add nextQuery property to getClusters/Instances response signature (it will alway be empty) and bring the response to a consistent across the repo form of err, objects, failedLocations, nextQuery, apiResponse.
Otherwise documenting where failed locations could be found consistency wise I think is a better option. What do you think?

The inconsistency comes from the fact that next_page_token is deprecated from listClusters/InstancesResponse, which makes nextQuery obsolete for those functions.

Yeah, and that's fine, so nothing we can do there. Having a new callback parameter "failedLocations" was a good idea 👍

But just so I understand here, this method does have pagination, right? "nextPageRequest" is basically "nextQuery"?

Otherwise documenting where failed locations could be found consistency wise I think is a better option.

I would lean towards the documentation option. I think it would mislead users to return the defunct nextQuery.

"nextPageRequest" is basically "nextQuery"?

nextPageRequest is the IListObjectsRequest in this case IListAppProfilesRequest.
The nextPageRequest is only returned when there are more results available.

I would lean towards the documentation option. I think it would mislead users to return the defunct nextQuery.

Empty object for nextQuery/nextPageRequest also signals that there are no more results to be returned. I won't disrupt the workflow
something like

      const gaxOptions = {autoPaginate: false};
      let allObjects: = [];
      let getObjectsOptions = {
        pageSize: 1,
        pageToken: '',
        gaxOptions,
      };
      while (true) {
        const [objects, , next] = await instance.getObjects(
          getObjectsOptions
        );
        allObjects = [...allObjects, ...objects];
        if (!next) {
          break;
        }
        getObjectsOptions = Object.assign(
          {},
          {gaxOptions},
          {pageSize: next.pageSize!, pageToken: next.pageToken!}
        );
      }

WDYT?

The nextPageRequest is only returned when there are more results available.

Will the user just provide that argument directly, like they do our nextQuery object?:

getAppProfiles(config, function(err, appProfiles, nextQuery) {
  if (nextQuery) {
    getAppProfile(nextQuery, function(err, nextAppProfiles, nextNextQuery) {})
  }
})

Our nextQuery object should always be an extension of the user-provided c onfig with the appropriate settings for cursoring. We should stick to that system, if we're not already, by passing the IListAppProfilesRequest back.

Empty object for nextQuery/nextPageRequest also signals that there are no more results to be returned.

That's true, but it's misleading to make users write code anticipating more results could exist, when we know they wouldn't ever. That's also a little confusing to maintain down the road. The main thing I'm avoiding is the high number of response arguments. We've usually tried to consolidate the response pattern:

• error
• the most logical thing the user expects
• the full API response for whenever they need something else

For query methods, we added in "nextQuery" to make cursoring easier:

• error
• the most logical thing the user expects
• an object ready to pass to the API to continue getting results
• the full API response for whenever they need something else

So, I'm hesitant to squeeze one more in there, as if it's not "the most logical thing the user expects", then we've historically pointed them to "the full API response for whenever they need something else".

One more thought-- if the user runs this with autoPagination: true (which is on by default), doesn't that abolish all apiResponses? In this sample:

getAppProfiles()
  .then(result => {
    const appProfiles = result[0];
    if (result[1]) {
      // These locations contain instances which could not be retrieved.
      const failedLocations = result[1];
    }
  })

What actually is failedLocations? An array of arrays of strings?

My vote would go to documenting how to find failedLocations in the API response, but more input on this one could be useful.

Our nextQuery object should always be an extension of the user-provided config with the appropriate settings for cursoring. We should stick to that system, if we're not already, by passing the IListAppProfilesRequest back.

Combined IListAppProfilesRequest with user-provided config, now below sample will work.

getAppProfiles(config, function(err, appProfiles, nextQuery) {
  if (nextQuery) {
    getAppProfile(nextQuery, function(err, nextAppProfiles, nextNextQuery) {})
  }
})

My vote would go to documenting how to find failedLocations in the API response, but more input on this one could be useful.

Removed failedLocations from the response and documented in the callback section instead.

Final take in regards to getInstances and getClusters response/callback. Both methods deprecated next_page_token. How shall the callback look {err, resources, failedLocations, fullResponse} or just {err, resources, fullResponse} and document where to find failedLocations if any?

PTAL

How shall the callback look {err, resources, failedLocations, fullResponse} or just {err, resources, fullResponse} and document where to find failedLocations if any?

Very good question. I think there's a good case for both options. Have we released yet with those changes, where if we change the response signature, we'd be breaking? If we have to break, I'd probably just leave those alone. If not, I would choose (err, resources, apiResponse):

• 👍 Simple signature
• 👍 Consistent with most of our methods, where we return "error, thing you want, everything you could ever want"
• 👎 Have to find failedLocations inside of apiResponse

Great, will revert to same in bigtable.getInstances()

Sorry if I have overlooked this before, but is autoPaginate not a top-level option anymore? Does the user have to provide this through options.gaxOptions.autoPaginate? I think we should allow options.autoPaginate, in the same way we special-case options.pageSize,

I followed example that was done in the Spanner repo googleapis/nodejs-spanner#855 (comment)

WDYT?

That was a pretty big PR to review, and it looks like I had too many streams of conversations to figure out how they all pieced together :\ . In that case, did we get rid of "options.autoPaginate", but keep "options.pageSize"? That simply doesn't make sense, as they are both related options, and in order to give meaning to "pageSize", setting "autoPaginate" to "false" is required. Fortunately, we should be able to add that property back in to Spanner relatively easily, or alternatively, remove all other top-level pagination options.

So for here let's go with the top level autoPaginate option and have API handle it the same way as pageSize and pageToken

Cool, sounds good to me. Thank you!

Added top level autoPaginate to getAppProfiles and getTables.