Summary: | results.webkit.org: Add database table to save zip archives to | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jonathan Bedard <jbedard> | ||||||||
Component: | Tools / Tests | Assignee: | Jonathan Bedard <jbedard> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | aakash_jain, commit-queue, dean_johnson, kocsen_chung, lforschler, webkit-bug-importer, zhifei_fang | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=200690 | ||||||||||
Attachments: |
|
Description
Jonathan Bedard
2019-08-14 09:02:49 PDT
Created attachment 376270 [details]
Patch
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. 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. Created attachment 376884 [details]
Patch
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. 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 Created attachment 376923 [details]
Patch
Comment on attachment 376923 [details] Patch Clearing flags on attachment: 376923 Committed r248979: <https://trac.webkit.org/changeset/248979> All reviewed patches have been landed. Closing bug. |