Skip to content
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

feat(batch-exports): Backfill UI #28146

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open

feat(batch-exports): Backfill UI #28146

wants to merge 17 commits into from

Conversation

rossgray
Copy link
Contributor

@rossgray rossgray commented Jan 31, 2025

Problem

Currently the only way to view batch export backfills is via the API. It should be possible to view backfills in the UI.

Closes #27788

Changes

Frontend

  • Add a 'Backfills` tab to the Data pipeline -> Destinations page.
image
  • Move the 'Backfill batch export' button + modal from the Runs scene to Backfills. On the runs scene the button is still there is there are no runs. Therefore the backfill modal logic has been extracted out of batchExportRunsLogic.tsx and into its own batchExportBackfillsLogic.tsx.
image
  • There is a button to cancel an in-progress backfill

Backend

  • Add a DRF ModelViewSet for backfills
    • You can fetch a list of backfills, fetch an individual backfill
    • Creation of backfills already existed
  • Previously we were never setting the value for the backfill finished_at column, so have made sure we do this going forwards

Does this work well for both Cloud and self-hosted?

Should do.

How did you test this code?

Added automated tests.

Have also tested manually.

Copy link
Contributor

github-actions bot commented Jan 31, 2025

Size Change: +399 B (+0.03%)

Total Size: 1.17 MB

ℹ️ View Unchanged
Filename Size Change
frontend/dist/toolbar.js 1.17 MB +399 B (+0.03%)

compressed-size-action

@rossgray rossgray requested a review from tomasfarias January 31, 2025 17:05
@rossgray rossgray marked this pull request as ready for review January 31, 2025 17:44
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Summary

This PR adds a dedicated UI for managing batch export backfills, moving from API-only access to a full user interface. Here's a summary of the key changes:

  • Added new 'Backfills' tab in Data Pipeline -> Destinations page with a table view showing backfill status, timestamps, and controls
  • Extracted backfill modal logic from batchExportRunsLogic into dedicated batchExportBackfillModalLogic for better code organization
  • Added backend API endpoints under /api/projects/{team_id}/batch_exports/{batch_export_id}/backfills/ with proper access controls
  • Fixed issue where backfill finished_at timestamp wasn't being set properly
  • Added ability to cancel in-progress backfills through the UI

Key implementation points:

  • New BatchExportBackfills component displays backfills with status indicators and cancel controls
  • batchExportBackfillModalLogic handles form validation with timezone-aware date handling
  • Backend changes include proper team/organization access controls and data partitioning
  • Added comprehensive test coverage for backfill operations including cancellation and status updates
  • Maintained backward compatibility while improving code organization

30 file(s) reviewed, 24 comment(s)
Edit PR Review Bot Settings | Greptile

cancelBackfill,
}: {
backfill: BatchExportBackfill
cancelBackfill: any
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: cancelBackfill should have a proper type definition instead of 'any'

Suggested change
cancelBackfill: any
cancelBackfill: (backfill: BatchExportBackfill) => void

const color = colorForStatus(status)
return (
<span
className={`h-6 p-2 border-2 flex items-center justify-center rounded-full font-semibold text-xs border-${color} text-${color}-dark select-none`}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: string interpolation in className may not work with CSS modules/purging - consider using a mapping object or utility classes

tooltip="Cancel backfill"
onClick={() =>
LemonDialog.open({
title: 'Cancel run?',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: dialog title 'Cancel run?' should be 'Cancel backfill?' to match the action being performed

footer={
hasMoreBackfillsToLoad && (
<div className="flex items-center m-2">
<LemonButton center fullWidth onClick={loadOlderBackfills} loading={loading}>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: loading prop is used while loadOlderBackfills is in progress, but loading state is shared with other operations which could cause UI issues

Comment on lines +73 to +74
(start_at?.minute() !== undefined && !(start_at?.minute() % 5 === 0)) ||
(end_at?.minute() !== undefined && !(end_at?.minute() % 5 === 0))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Potential edge case: start_at?.minute() could be undefined but end_at?.minute() defined or vice versa. Consider handling these cases separately for clearer error messages.

def test_connot_pause_and_unpause_batch_exports_of_other_organizations(client: HttpClient):
temporal = sync_connect()

def test_connot_pause_and_unpause_batch_exports_of_other_organizations(client: HttpClient, temporal):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

syntax: Typo in function name: 'connot' should be 'cannot'

Suggested change
def test_connot_pause_and_unpause_batch_exports_of_other_organizations(client: HttpClient, temporal):
def test_cannot_pause_and_unpause_batch_exports_of_other_organizations(client: HttpClient, temporal):

Comment on lines 150 to +151
@pytest.mark.django_db(transaction=True)
def test_cancelling_a_batch_export_run(client: HttpClient):
def test_cancelling_a_batch_export_run(client: HttpClient, temporal):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: The transaction=True decorator is redundant since the test is already marked with pytestmark django_db

Comment on lines +303 to +306
if not self.start_at or not self.end_at:
# just return None in this case since we don't have enough information to calculate the total runs
return None
return (self.end_at - self.start_at) // self.batch_export.interval_time_delta
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: The integer division (//) may truncate partial intervals. Consider rounding up with math.ceil() to ensure all partial intervals are counted.

Comment on lines +808 to +810
def update_batch_export_backfill_status(
backfill_id: UUID, status: str, finished_at: dt.datetime | None = None
) -> BatchExportBackfill:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: finished_at parameter added but not used in async version aupdate_batch_export_backfill_status. Should be consistent between sync/async versions.

Suggested change
def update_batch_export_backfill_status(
backfill_id: UUID, status: str, finished_at: dt.datetime | None = None
) -> BatchExportBackfill:
async def aupdate_batch_export_backfill_status(backfill_id: UUID, status: str, finished_at: dt.datetime | None = None) -> BatchExportBackfill:
"""Update the status of an BatchExportBackfill with given id.
Arguments:
id: The id of the BatchExportBackfill to update.
status: The new status to assign to the BatchExportBackfill.
finished_at: The time the BatchExportBackfill finished.
"""
model = BatchExportBackfill.objects.filter(id=backfill_id)
updated = await model.aupdate(status=status, finished_at=finished_at)
if not updated:
raise ValueError(f"BatchExportBackfill with id {backfill_id} not found.")
return await model.aget()

Comment on lines 894 to 901
async def update_batch_export_backfill_model_status(inputs: UpdateBatchExportBackfillStatusInputs) -> None:
"""Activity that updates the status of an BatchExportRun."""
backfill = await database_sync_to_async(update_batch_export_backfill_status)(
backfill_id=uuid.UUID(inputs.id), status=inputs.status
backfill_id=uuid.UUID(inputs.id),
status=inputs.status,
# we currently only call this once the backfill is finished, so we can set the finished_at here
finished_at=dt.datetime.now(dt.UTC),
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Setting finished_at here assumes the activity is only called when the backfill is finished. Consider adding a parameter to explicitly control when to set finished_at to avoid potential bugs if the activity is called for intermediate status updates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Batch export: Backfill UI
1 participant