Bug 200718 – results.webkit.org: Add database table to save zip archives to

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Jonathan Bedard]

We should have a table in Cassandra which stores zip archives associated with a test run.

[Jonathan Bedard]

Created attachment 376270 [details]
Patch

[Kocsen Chung]

Comment on attachment 376270 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=376270&action=review

> Tools/resultsdbpy/resultsdbpy/model/archive_context.py:101
> +                    ttl=int((uuid // Commit.TIMESTAMP_TO_UUID_MULTIPLIER) + self.ttl_seconds - time.time()) if self.ttl_seconds else None,

Seems repeated code from initial `ttl` declaration above.

[Jonathan Bedard]

Comment on attachment 376270 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=376270&action=review

>> Tools/resultsdbpy/resultsdbpy/model/archive_context.py:101
>> +                    ttl=int((uuid // Commit.TIMESTAMP_TO_UUID_MULTIPLIER) + self.ttl_seconds - time.time()) if self.ttl_seconds else None,
> 
> Seems repeated code from initial `ttl` declaration above.

It would seem like that....I'll change it.

[Jonathan Bedard]

Created attachment 376884 [details]
Patch

[Aakash Jain]

Comment on attachment 376884 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=376884&action=review

rs=me

> Tools/resultsdbpy/resultsdbpy/model/archive_context.py:37
> +    DEFAULT_LIMIT = 10

DEFAULT_LIMIT for? Can rename it to be more expressive.

> Tools/resultsdbpy/resultsdbpy/model/archive_context.py:74
> +            self.cassandra.create_table(self.ArchivesByCommit)

Just to confirm, we won't be creating a new table each time we instantiate this class, right?

> Tools/resultsdbpy/resultsdbpy/model/archive_context.py:89
> +        if isinstance(timestamp, datetime):

can we use the get_time() method here as well?

> Tools/resultsdbpy/resultsdbpy/model/archive_context.py:90
> +            timestamp = calendar.timegm(timestamp.timetuple())

why do we use timestamp.timetuple() here but not inside get_time() method?

> Tools/resultsdbpy/resultsdbpy/model/archive_context.py:119
> +        def get_time(time):

you might consider making this stand-alone method(_get_time()), outside find_archive(). Also renaming the parameter 'time' to something like 'input_time' might be more clear.

> Tools/resultsdbpy/resultsdbpy/model/archive_context.py:122
> +            elif time:

should convert this elif to if, since previous if has a corresponding 'return' statement.

> Tools/resultsdbpy/resultsdbpy/model/mock_model_factory.py:185
> +            old = current - 60 * 60 * 24 * 21

can we store 60 * 60 * 24 * 21 in a variable to make it more readable.

[Jonathan Bedard]

Comment on attachment 376884 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=376884&action=review

>> Tools/resultsdbpy/resultsdbpy/model/archive_context.py:74
>> +            self.cassandra.create_table(self.ArchivesByCommit)
> 
> Just to confirm, we won't be creating a new table each time we instantiate this class, right?

Correct. The CassandraContext class is smart enough to only create the table if it doesn't exist.

>> Tools/resultsdbpy/resultsdbpy/model/archive_context.py:90
>> +            timestamp = calendar.timegm(timestamp.timetuple())
> 
> why do we use timestamp.timetuple() here but not inside get_time() method?

Because the get_time method is wrong....calendar.timegm(...) requires the tuple

[Jonathan Bedard]

Created attachment 376923 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 376923 [details]
Patch

Clearing flags on attachment: 376923

Committed r248979: <https://trac.webkit.org/changeset/248979>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/54579632>