Details

Reviewers

vlorentz
zack

Group Reviewers

Reviewers

Commits

rDSTOC2ead4ce360ba: Added comments for all tables and columns
rDSTO2ead4ce360ba: Added comments for all tables and columns

Summary

Related T1527

Diff Detail

Repository

rDSTO Storage manager

Branch

sql-table-column-comments

Lint

No Linters Available

Unit

No Unit Test Coverage

Build Status

Buildable 6300
Build 8721: tox-on-jenkins	Jenkins
Build 8720: arc lint + arc unit

Event Timeline

twitu created this revision.Jun 14 2019, 8:19 AM

Herald added a reviewer: Reviewers. · View Herald TranscriptJun 14 2019, 8:19 AM

This diff is in reference to T1527.

Build is green
See https://jenkins.softwareheritage.org/job/DSTO/job/tox/483/ for more details.

Harbormaster completed remote builds in B6205: Diff 5231.Jun 14 2019, 8:26 AM

twitu mentioned this in T1527: Have comments on all columns of all databases.Jun 14 2019, 8:30 AM

sha1_git is not "Git id of sha1 checksum", it's the hash of the content as a git object (ie. with the header used by git to identify blobs).

Could you also add comments on these tables, and use is instead of IS?

swh/storage/sql/30-swh-schema.sql
62	Not "file creation", but the first time we saw that content.
64	It's not an identifier of the "file content", just of the content
82–84	I don't think comments on these columns are relevant. Maybe just one of the type.
113	Same
140	This comment should go into more details

This revision now requires changes to proceed.Jun 14 2019, 3:28 PM

zack requested changes to this revision.Jun 14 2019, 3:28 PM

zack added a subscriber: zack.

zack added inline comments.

swh/storage/sql/30-swh-schema.sql
57–64	the style of these could be improved to be less awkward, e.g., "length of file content" -> "file content length" also, please check our glossary, as it will give hints of how to make this both shorter and clearer, e.g., it's not "file content", it's just "content"
141–142	these are incorrect, you should just refer to the general notions of "standard output / standard error", the intended reader will know what they mean

twitu added inline comments.Jun 14 2019, 5:30 PM

swh/storage/sql/30-swh-schema.sql
57–64	I prefer using active voice for comments. But the comments will become repetitive like, Content length Content seen time Content identifier

ardumont edited the summary of this revision. (Show Details)Jun 14 2019, 5:43 PM

Added comments for all tables and columns

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DSTO/job/tox/492/
See console output for more information: https://jenkins.softwareheritage.org/job/DSTO/job/tox/492/console

Harbormaster failed remote builds in B6232: Diff 5256!Jun 14 2019, 9:24 PM

twitu added inline comments.Jun 14 2019, 9:29 PM

sql/upgrades/137.sql
87	The db fields do not give information to add comments. Can you give some information about what is being stored. Specially fields like date offset.
106	Not sure what to write here. Is is the complete revision history of a software origin?
134	What to add here for date offset.
152	Is there a better comment than "Primary key" for this field.
154	The dev comment says identifier across which we're bucketing objects Does this comment convey the meaning?

@twitu please use imperative form while writing commit messages. Also, do keep in mind the why-not-how when writing the body of the commit message.

Add comments to few columns in dbversion, task and task_run
Change double quoted comments to single quoted comments

Build is green
See https://jenkins.softwareheritage.org/job/DSCH/job/tox/270/ for more details.

Harbormaster completed remote builds in B6236: Diff 5260.Jun 15 2019, 7:20 PM

Added comments to tables dbversion, content, skipped_content and fetch_history
Added comments for all tables and columns
Converted double quote comments to single quote comments

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DSTO/job/tox/494/
See console output for more information: https://jenkins.softwareheritage.org/job/DSTO/job/tox/494/console

Harbormaster failed remote builds in B6239: Diff 5263!Jun 15 2019, 7:36 PM

fyi, the error is:

---------------------------- Captured stderr setup -----------------------------
psql:/home/jenkins/workspace/DSTO/tox/swh/storage/sql/30-swh-schema.sql:110: NOTICE:  identifier "Content blobs observed but not ingested in
 Software heritage archive" will be truncated to "Content blobs observed but not ingested in
 Software heritage a"
psql:/home/jenkins/workspace/DSTO/tox/swh/storage/sql/30-swh-schema.sql:110: ERROR:  syntax error at or near ""Content blobs observed but not ingested in
 Software heritage archive""
LINE 1: comment on table skipped_content is "Content blobs observed ...
                                            ^

The comments are not complete. I will fix the syntax issue in the next commit along with the missing comments. I am not sure what comments to add for the revision table.

Add additional column comments

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DSTO/job/tox/499/
See console output for more information: https://jenkins.softwareheritage.org/job/DSTO/job/tox/499/console

Harbormaster failed remote builds in B6264: Diff 5285!Jun 17 2019, 6:07 PM

Double quote syntax error

Build is green
See https://jenkins.softwareheritage.org/job/DSTO/job/tox/500/ for more details.

Harbormaster completed remote builds in B6266: Diff 5287.Jun 17 2019, 7:00 PM

ardumont retitled this revision from Added comments to tables dbversion, content, skipped_content and fetch_history to Add comments to tables dbversion, content, skipped_content and fetch_history.Jun 18 2019, 10:26 AM

Does this diff require any other changes?

vlorentz added inline comments.Jun 18 2019, 2:07 PM

sql/upgrades/137.sql
12	not release, but deployment
85	Should also mention that it's the "raw" name for most CVSs
swh/storage/sql/30-swh-schema.sql
14	not release, but deployment

Make suggested changes

List all options

twitu marked 7 inline comments as done.Jun 18 2019, 4:39 PM

twitu added inline comments.

swh/storage/sql/30-swh-schema.sql
82–84	No point deleting them now.

Build is green
See https://jenkins.softwareheritage.org/job/DSTO/job/tox/501/ for more details.

Harbormaster completed remote builds in B6299: Diff 5315.Jun 18 2019, 4:40 PM

Build is green
See https://jenkins.softwareheritage.org/job/DSTO/job/tox/502/ for more details.

Harbormaster completed remote builds in B6300: Diff 5316.Jun 18 2019, 4:53 PM

Almost there !
(and thanks a lot for your persistence on this one)

sql/upgrades/137.sql
11	SQL (uppercase) there are a few other instances of this below, please fix all of them
swh/storage/sql/30-swh-schema.sql
85	URL (uppercase)
110	We can leave "software heritage" implicit, given the context. How about "Content blobs observed, but not ingested in the archive" here?
144	I don't think my previous comment on these fields have actually been addressed. My proposal is something like "standard output of the fetch operation" / "standard error of the fetch operation".