owncloud / ocis

:atom_symbol: ownCloud Infinite Scale Stack
https://doc.owncloud.com/ocis/next/
Apache License 2.0
1.4k stars 182 forks source link

fetchShares in the sharesstorageprovider does not parallelize stat requests #9828

Closed butonic closed 4 days ago

butonic commented 2 months ago

looking at the code ... we are sequentially stating all accepted shares. this will take ages and can be parallelized. maybe even cached

func (s *service) fetchShares(ctx context.Context, opaque *typesv1beta1.Opaque, arbitraryMetadataKeys []string, fieldMask *field_mask.FieldMask) ([]*collaboration.ReceivedShare, map[string]*provider.ResourceInfo, error) {
    sharingCollaborationClient, err := s.sharingCollaborationSelector.Next()
    if err != nil {
        return nil, nil, err
    }

    lsRes, err := sharingCollaborationClient.ListReceivedShares(ctx, &collaboration.ListReceivedSharesRequest{
        // FIXME filter by received shares for resource id - listing all shares is tooo expensive!
    })
    if err != nil {
        return nil, nil, errors.Wrap(err, "sharesstorageprovider: error calling ListReceivedSharesRequest")
    }
    if lsRes.Status.Code != rpc.Code_CODE_OK {
        return nil, nil, fmt.Errorf("sharesstorageprovider: error calling ListReceivedSharesRequest")
    }

    gatewayClient, err := s.gatewaySelector.Next()
    if err != nil {
        return nil, nil, err
    }

    shareMetaData := make(map[string]*provider.ResourceInfo, len(lsRes.Shares))
    for _, rs := range lsRes.Shares {
        // only stat accepted shares
        if rs.State != collaboration.ShareState_SHARE_STATE_ACCEPTED {
            continue
        }
        if rs.Share.ResourceId.SpaceId == "" {
            // convert backwards compatible share id
            rs.Share.ResourceId.StorageId, rs.Share.ResourceId.SpaceId = storagespace.SplitStorageID(rs.Share.ResourceId.StorageId)
        }
        sRes, err := gatewayClient.Stat(ctx, &provider.StatRequest{
            Opaque:                opaque,
            Ref:                   &provider.Reference{ResourceId: rs.Share.ResourceId},
            ArbitraryMetadataKeys: arbitraryMetadataKeys,
            FieldMask:             fieldMask,
        })
        if err != nil {
            appctx.GetLogger(ctx).Error().
                Err(err).
                Interface("resourceID", rs.Share.ResourceId).
                Msg("ListRecievedShares: failed to make stat call")
            continue
        }
        if sRes.Status.Code != rpc.Code_CODE_OK {
            appctx.GetLogger(ctx).Debug().
                Interface("resourceID", rs.Share.ResourceId).
                Interface("status", sRes.Status).
                Msg("ListRecievedShares: failed to stat the resource")
            continue
        }
        shareMetaData[rs.Share.Id.OpaqueId] = sRes.Info
    }

    return lsRes.Shares, shareMetaData, nil
}

fix by concurrently stating resources. make the number of concurrent go routines configurable.

butonic commented 2 months ago

related, but bitrotted: https://github.com/cs3org/reva/pull/3940

butonic commented 2 months ago

fixed by https://github.com/cs3org/reva/pull/4812

butonic commented 2 months ago