[DEV-6356] installation progress #3564

Ildyakov · 2023-04-11T16:01:55Z

This change is

vmarkovtsev · 2023-04-13T09:55:22Z

server/athenian/api/models/state/models.py

@@ -640,3 +640,19 @@ class DashboardChartGroupBy(create_time_mixin(created_at=True, updated_at=True),
    DashboardChartGroupBy.jira_issue_types,
    DashboardChartGroupBy.jira_labels,
 )
+
+
+class InstallationProgress(create_time_mixin(created_at=False, updated_at=False), Base):


Suggested change

class InstallationProgress(create_time_mixin(created_at=False, updated_at=False), Base):

class GitHubInstallationProgress(Base):

I guess. If we write "github" in the description... So that we don't confuse with GitLab.
Also, create_time_mixin(created_at=False, updated_at=False) does nothing, right?

vmarkovtsev · 2023-04-13T10:01:06Z

server/athenian/api/models/state/models.py

+    account_created = Column(TIMESTAMP(timezone=True))
+    fetch_started = Column(TIMESTAMP(timezone=True))


How is "account_created" different from "fetch_started"?

more so we don`t have a "account_created" event. yet.
and for a case of re-fetch they will be different.
so for now we can cut this field I think. yes

vmarkovtsev · 2023-04-13T10:02:56Z

server/athenian/api/models/state/models.py

+    precompute_started = Column(TIMESTAMP(timezone=True))
+    precompute_completed = Column(TIMESTAMP(timezone=True))


This is reposet and Athenian account specific. We have to move those timestamps to the repository_sets table.

so this force us to have some key in the state DB for github_account_ids.
create a table for comparing github_account_id and athenian_account_id needed.

You probably mean account_github_accounts?

if it is contains both this IDs - yes it is

Then yes it is

vmarkovtsev · 2023-04-13T10:04:04Z

server/athenian/api/models/state/models.py

+    consistency_completed = Column(TIMESTAMP(timezone=True))
+    precompute_started = Column(TIMESTAMP(timezone=True))
+    precompute_completed = Column(TIMESTAMP(timezone=True))
+    current_status = Column(Text())


Let's delete this for now. I need us to create something absolutely minimal and ASAP. If we make the status, we will have to think what statuses can we have, when to update and what, consider different edge cases, etc.

noone is forcing us to use all the columns of the table from the begining.
we can create a table "as is" and add the features to the pipeline and API when we will be ready

Let's migrate to additional fields in the future. To avoid seduction.

Sorry, but how one text field is making it harder? We already have a PR to MD that uses it.

addition - PR to cloud-common is ready too. and if we cut the columns it will slow us to implement changes instead of getting some features faster.
and if we delete precompute timestamps from this table we will use all the rest immediately after releasing all the features that in process now

Therefore, we can update the status field in the metadata whatever way we want, but we will not use it downstream. If it warms your heart much to update an unused field - I am not standing against 😄

Yes, I'd still like to have that field for the bot.

Agree about precomputer, we will need a second table then?

precompute- fields dropped. account_created field dropped. others (including status) is used by actual written logic in metadata. the point is that status can have a voluntary (apart of some statuses that would be fixed) value and this can be used for some specific messages like "paused", "delayed" etc.

@dennwc we have a field "precomputed bool" in the table "repository_sets"
so it would be right to add a couple new fields to it. and provide a logic to fill this fields from the precomputer directly (not with event handler).

It makes sense to write to metadata 👍

vmarkovtsev · 2023-04-17T08:40:35Z

server/athenian/api/models/state/models.py

@@ -100,6 +100,8 @@ class RepositorySet(
    )
    tracking_re = Column(Text(), nullable=False, default=".*", server_default=".*")
    precomputed = Column(Boolean(), nullable=False, default=False, server_default="false")
+    precompute_started = Column(TIMESTAMP(timezone=True))
+    precompute_completed = Column(TIMESTAMP(timezone=True))


Suggested change

precompute_completed = Column(TIMESTAMP(timezone=True))

precompute_finished = Column(TIMESTAMP(timezone=True))

Minor syntax nitpick: start-finish; begin-end; initiate-complete. I know that people don't usually care, but this is just good education and conformance to the surrounding table conventions :)

Oh, and I see we have the same syntax to fix in installation_progress 🙏

vmarkovtsev · 2023-04-17T08:42:35Z

server/athenian/api/models/state/models.py

+    consistency_completed = Column(TIMESTAMP(timezone=True))
+    precompute_started = Column(TIMESTAMP(timezone=True))
+    precompute_completed = Column(TIMESTAMP(timezone=True))
+    current_status = Column(Text())


Therefore, we can update the status field in the metadata whatever way we want, but we will not use it downstream. If it warms your heart much to update an unused field - I am not standing against 😄

dennwc

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @Ildyakov and @vmarkovtsev)

server/athenian/api/models/state/versions/5b3dc49a9d7b_installation_progress.py line 20 at r2 (raw file):

def upgrade():
    op.create_table(
        "installation_progress",

Since we are removing API-related things from the table anyway, does it make sense to move it to MD repo completely? It can be a set of added columns for github.account, for example. This way we can add a data migration as well (set all accounts to "done" status).

Ildyakov · 2023-04-17T11:02:25Z

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @Ildyakov and @vmarkovtsev)

server/athenian/api/models/state/versions/5b3dc49a9d7b_installation_progress.py line 20 at r2 (raw file):
def upgrade():
    op.create_table(
        "installation_progress",
Since we are removing API-related things from the table anyway, does it make sense to move it to MD repo completely? It can be a set of added columns for github.account, for example. This way we can add a data migration as well (set all accounts to "done" status).

this is interesting. yes the reason to use state db was precomputing.
I think this is a proper solution to move the draft to metadata. WDYT @vmarkovtsev?

[DEV-6356] installation progress

09d8abd

Ildyakov requested a review from dennwc April 11, 2023 16:02

vmarkovtsev mentioned this pull request Apr 13, 2023

[DEV-6356] added table installation_progress #3547

Closed

vmarkovtsev requested changes Apr 13, 2023

View reviewed changes

[DEV-6356] updates for columns instaprogress

fadd50c

vmarkovtsev requested changes Apr 17, 2023

View reviewed changes

dennwc suggested changes Apr 17, 2023

View reviewed changes

aleksei added 2 commits April 20, 2023 08:33

[DEV-6356] move installation progress to metadata

affb9b8

[DEV-6356] flake update

a7999e5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DEV-6356] installation progress #3564

[DEV-6356] installation progress #3564

Ildyakov commented Apr 11, 2023 •

edited by eiso

Loading

vmarkovtsev Apr 13, 2023 •

edited

Loading

Ildyakov Apr 13, 2023

vmarkovtsev Apr 13, 2023

Ildyakov Apr 13, 2023

vmarkovtsev Apr 13, 2023

Ildyakov Apr 13, 2023

vmarkovtsev Apr 13, 2023

Ildyakov Apr 14, 2023

vmarkovtsev Apr 17, 2023

vmarkovtsev Apr 13, 2023

Ildyakov Apr 13, 2023

vmarkovtsev Apr 13, 2023

dennwc Apr 14, 2023

Ildyakov Apr 14, 2023

vmarkovtsev Apr 17, 2023

dennwc Apr 17, 2023

Ildyakov Apr 17, 2023

Ildyakov Apr 17, 2023

vmarkovtsev Apr 17, 2023

vmarkovtsev Apr 17, 2023

vmarkovtsev Apr 17, 2023

vmarkovtsev Apr 17, 2023

dennwc left a comment

Ildyakov commented Apr 17, 2023

	class InstallationProgress(create_time_mixin(created_at=False, updated_at=False), Base):
	class GitHubInstallationProgress(Base):

		account_created = Column(TIMESTAMP(timezone=True))
		fetch_started = Column(TIMESTAMP(timezone=True))

		precompute_started = Column(TIMESTAMP(timezone=True))
		precompute_completed = Column(TIMESTAMP(timezone=True))

	precompute_completed = Column(TIMESTAMP(timezone=True))
	precompute_finished = Column(TIMESTAMP(timezone=True))

[DEV-6356] installation progress #3564

Are you sure you want to change the base?

[DEV-6356] installation progress #3564

Conversation

Ildyakov commented Apr 11, 2023 • edited by eiso Loading

vmarkovtsev Apr 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dennwc left a comment

Choose a reason for hiding this comment

Ildyakov commented Apr 17, 2023

Ildyakov commented Apr 11, 2023 •

edited by eiso

Loading

vmarkovtsev Apr 13, 2023 •

edited

Loading