4
\$\begingroup\$

I have a scheduled task where I synch all the data from one table (via an API) to another. There are about 10,000 rows that are being updated and the new ones get created. This is what I came up with and for some reason, it is extremely slow.

def sync_data_from_hub():
    stats = tasks.sync_clients_from_hub()

    return stats

tasks.py

# 1. This method calls an external API to fetch all the rows.
def sync_clients_from_hub():
    url = resolve_hub_url('clients')

    response = requests.get(url, headers=headers)

    if response.status_code != 200:
        print('api response error: {}'.format(response))
        return

    try:
        response_payload = response.json()
    except ValueError:
        return

    # response from API
    clients_data = response_payload.get('data')

    for (key, value) in clients_data.items():
        # Check whether there are any actual changes in this row 
        # by comparing the last updated date
        if services.should_update_data_from_hub(value, constants.SyncTypes.CLIENT):
            user, action = services.update_or_create_client_from_hub_data(value)

            if action == constants.SyncActions.CRT:
                result['new'] += 1
            if action == constants.SyncActions.UPD:
                result['update'] += 1

    # soft delete clients which are not present in payload but still available in database.
    if clients_data:
        discarded_clients = models.Client.objects.exclude(pk__in=list(map(int, clients_data.keys()))).filter(
            is_active=True)

        for client in discarded_clients:
            client.is_active = False
            client.save()

            result['deleted'] += 1

    return result

service.py

# logic to determine whether we should update the row or not
def should_update_data_from_hub(hub_data: dict, sync_type: str) -> bool:
    last_updated_date_from_api = hub_data.get('updatedAt', None)

    hub_data['id'] = int(hub_data.get('client', ''))

    # This line hits DB to fetch the row corresponding to the one in the API
    entity = get_client_by_pk(hub_data['id'])

    if not entity:
        return True

    last_updated_date_local = entity.last_updated_date

    if not last_updated_date_local:
        return True
    # The last updated dates have different timezones, so I have to adjust the timezone before comparison.
    last_updated_date_tz = last_updated_date.replace(tzinfo=timezone.utc).astimezone(tz=None)

    last_updated_date_local_formatted = last_updated_date_tz.strftime('%Y-%m-%d %H:%M:%S')

    return last_updated_date_formatted != last_updated_date_from_api

# Actual method to update the data
def update_or_create_client_from_hub_data(client_data: dict):
    client_data['id'] = int(client_data.get('client', None))

    # Set `is_active` based on the `archived` flag.
    if client_data.get('archive', None) == '1':
        client_data['is_active'] = False
    else:
        client_data['is_active'] = True
    ...

    # Set `last_updated_date`
    client_data['last_updated_date'] = client_data.get('updatedAt', datetime.datetime.now())

    serializer = serializers.ClientCreateSerializer(data=client_data)

    if not serializer.is_valid():
        print('client sync errors: {}'.format(serializer.errors))
        return None, None

    # This line hits DB to fetch the row corresponding to the one in the API
    client = get_client_by_pk(serializer.validated_data['id'])

    if client:
        action = constants.SyncActions.UPD

        for (key, value) in serializer.validated_data.items():
            setattr(client, key, value)

        client.save()
    else:
        action = constants.SyncActions.CRT

        client = models.Client.objects.create(**serializer.validated_data)

    return client, action

def get_client_by_pk(pk: int) -> Optional[models.Client]:
    return models.Client.objects.filter(pk=pk).first()

How can I optimize it to perform better? Should I load all the rows from DB in memory first rather than fetching them one by one and doing a comparison?

\$\endgroup\$
  • 3
    \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Vogel612 Oct 15 '19 at 11:43
3
\$\begingroup\$

Error handling

if response.status_code != 200:
    print('api response error: {}'.format(response))
    return

is better represented as a simple call to response.raise_for_status().

Also, this:

try:
    response_payload = response.json()
except ValueError:
    return

is dangerous. Silently failing should (at the least) be left up to the caller to decide whether an exception should be caught and swallowed, caught and printed, or allowed to fall through.

Tuple unpacking

for (key, value) in clients_data.items():

can be

for key, value in clients_data.items():

Undeclared variable

        if action == constants.SyncActions.CRT:
            result['new'] += 1
        if action == constants.SyncActions.UPD:
            result['update'] += 1

Where does result come from? You don't show this. Is it a defaultdict?

Incorrect default

hub_data['id'] = int(hub_data.get('client', ''))

will not work in the case that the key is missing. You can't convert '' to an int. Perhaps you meant:

hub_data['id'] = int(hub_data.get('client', '0'))

Ellipses

This is more of a meta-comment, but

# Set `is_active` based on the `archived` flag.
if client_data.get('archive', None) == '1':
    client_data['is_active'] = False
else:
    client_data['is_active'] = True
...

should show the ellipses as a comment to avoid breaking syntactical validity, and you should provide rationale as to why this code is elided. Eliding code harms the potential for you to receive a meaningful review.

Optimization

How can I optimize it to perform better?

It's nearly impossible to say. You haven't run a profiler, which you should; you've elided too much code for us to run this thing; you haven't shown any example data; and you haven't given us an API endpoint (or if that's not possible, at least a sample of a payload the endpoint would return).

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.