Generate, index and display Dart SDK API results in pub search. #1581
Conversation
|
Staging is being updated with this new version, should be available in less then an hour. |
isoos
force-pushed
the
search_generate_sdk
branch
4 times, most recently
from
d0f0e45 to
4cf9faf
Compare
app/bin/service/search.dart
Outdated
| registerPackageIndex(new SimplePackageIndex()); | ||
| registerTaskSendPort(taskReceivePort.sendPort); | ||
|
|
||
| // don't block on SDK index updates, as it may take several minutes before | ||
| // the dartdoc service produces the required output. | ||
| _updateDartSdkIndex(); |
There was a problem hiding this comment.
I believe there is an optional hint about unawaited futures. This code also doesn't look like it creates a future. How about .then((_) {}) or something to show that we do wish to ignore this background future?
app/bin/service/search.dart
Outdated
| _logger.warning('Error loading Dart SDK index.', e, st); | ||
| } | ||
| if (i % 10 == 0) { | ||
| _logger.warning('Unable to load Dart SDK index. Cycle: $i'); |
There was a problem hiding this comment.
Maybe use the word 'Attempt' instead of 'Cycle'
app/lib/dartdoc/backend.dart
Outdated
| @@ -43,6 +46,60 @@ class DartdocBackend { | |||
|
|
|||
| DartdocBackend(this._db, this._storage); | |||
|
|
|||
| /// Whether the storage bucket has a useable extracted data file. | |||
| try { | ||
| final info = await _storage.info(objectName); | ||
| return info != null; | ||
| } catch (_) { |
There was a problem hiding this comment.
Can we catch a specific exception instead?
There was a problem hiding this comment.
I recall two distinct kind of error there, but one of them may be related to the outage I have mentioned earlier.
There was a problem hiding this comment.
The other is DetailedApiRequestError, and I'm not sure where it is coming from.
app/lib/dartdoc/backend.dart
Outdated
| @@ -43,6 +46,60 @@ class DartdocBackend { | |||
|
|
|||
| DartdocBackend(this._db, this._storage); | |||
|
|
|||
| /// Whether the storage bucket has a useable extracted data file. | |||
| /// Only the existence of the file is checked. | |||
| // TODO: decide whether we should re-generate the file after a certain age | |||
There was a problem hiding this comment.
Is it redone when we bump pana or the runtime versions?
app/lib/dartdoc/backend.dart
Outdated
| storage_path.dartSdkDartdocDataName(shared_versions.runtimeVersion); | ||
| try { | ||
| final contentBytes = await file.readAsBytes(); | ||
| await _storage.writeBytes(objectName, _gzip.encode(contentBytes)); |
There was a problem hiding this comment.
Is there a stream way to write the bytes?
app/lib/dartdoc/backend.dart
Outdated
| } catch (e, st) { | ||
| final message = | ||
| 'Unable to read SDK pub dartdoc data file (attempt #${i + 1}): $objectName '; | ||
| _logger.info(message, e, st); |
There was a problem hiding this comment.
Woah. This seems flaky. What's the problem here?
There was a problem hiding this comment.
IIRC it was about the time where we have experienced several timeouts and 500s from the dartdoc's storage bucket both in prod and in staging. We can be more optimistic now, shall I remove?
There was a problem hiding this comment.
If you think it's stable? If we try once here, and it fails, is it later retried at a higher level?
There was a problem hiding this comment.
Yes, it is retried at a higher level, removing the redundancy.
app/lib/dartdoc/dartdoc_runner.dart
Outdated
| 'dart', | ||
| ['bin/pub_dartdoc.dart']..addAll(args), | ||
| workingDirectory: _pkgPubDartdocDir, | ||
| timeout: _dartdocTimeout * 2, |
There was a problem hiding this comment.
Generating docs for the SDK takes longer time, and this seemed to be a good number. I'll refactor it into a separate const.
| final isOk = pr.exitCode == 0 && hasPubData; | ||
| if (!isOk) { | ||
| _logger.warning( | ||
| 'Error while generating SDK docs.\n\n${pr.stdout}\n\n${pr.stderr}'); |
There was a problem hiding this comment.
Should this be an error? Can this be triggered by poor user code? In that case, can we recognize the difference between bad user input and an infrastructure problem?
There was a problem hiding this comment.
It is most likely an infrastructure problem, making it a hard error.
|
|
||
| /// The ObjectName for the Dart SDK's extracted dartdoc content. | ||
| String dartSdkDartdocDataName(String runtimeVersion) { | ||
| return sdkObjectName('dart/pub-dartdoc-data/$runtimeVersion.json.gz'); |
There was a problem hiding this comment.
Does anything garbage collect these old files or do they accumulate indefinitely? Does this mean we have to update the runtime version whenever we update the Dart SDK? Should we decouple the Dart SDK used for the API results and the Dart SDK pub's website itself uses?
There was a problem hiding this comment.
Does anything garbage collect these old files or do they accumulate indefinitely?
No GC yet, but there will be.
Does this mean we have to update the runtime version whenever we update the Dart SDK? Should we decouple the Dart SDK used for the API results and the Dart SDK pub's website itself uses?
We do need to update it right now, but we could decouple the runtime SDK version from it. I'll need to think about it if there is any additional risk with the current stack, and I'll file a separate PR.
isoos
force-pushed
the
search_generate_sdk
branch
from
4cf9faf to
0ec82a7
Compare
#181