ferenc-csaky commented on code in PR #27552:
URL: https://github.com/apache/flink/pull/27552#discussion_r2799018441
##########
flink-runtime-web/web-dashboard/src/app/pages/job/overview/watermarks/job-overview-drawer-watermarks.component.ts:
##########
@@ -54,6 +67,31 @@ export class JobOverviewDrawerWatermarksComponent implements
OnInit, OnDestroy {
public virtualItemSize = 36;
public readonly narrowLogData = typeDefinition<WatermarkData>();
+ // Timezone related properties
+ // Using IANA timezone names to properly handle Daylight Saving Time (DST)
+ // Reference: https://en.wikipedia.org/wiki/List_of_tz_database_time_zones
+ public timezoneOptions = [
Review Comment:
Modern browsers support `Intl.supportedValuesOf('timeZone')` which will
return a complete timezone list, so I'd rather use that. Or if the browser does
not support that, I'd restrict this behavior to borwser local or UTC.
I don't think a curated list can be a good solution here, cause by what
criteria we select that "few"? Including tech oriented regions would be
obvious, sure, but that does not help on someone who would require another TZ.
##########
flink-runtime-web/web-dashboard/src/app/components/humanize-watermark.pipe.ts:
##########
@@ -36,16 +37,68 @@ export class HumanizeWatermarkPipe implements PipeTransform
{
}
@Pipe({
- name: 'humanizeWatermarkToDatetime'
+ name: 'humanizeWatermarkToDatetime',
+ standalone: true,
+ pure: false
Review Comment:
Why `pure: false`? The template already passes `selectedTimezone` as an
argument. A pure pipe will re-run when either `watermark.watermark` or
`selectedTimezone` changes, so the impure pipe seems to be an unnecessary
overhead.
##########
flink-runtime-web/web-dashboard/src/app/pages/job/job-detail/status/job-status.component.less:
##########
Review Comment:
This file has no relevant change, just a reorder. Let's leave it as it was
then.
##########
flink-runtime-web/web-dashboard/src/app/pages/job/overview/watermarks/job-overview-drawer-watermarks.component.ts:
##########
@@ -62,7 +100,28 @@ export class JobOverviewDrawerWatermarksComponent
implements OnInit, OnDestroy {
private readonly cdr: ChangeDetectorRef
) {}
+ private getBrowserTimezone(): string {
Review Comment:
By default I'd keep browser local instead of UTC, no matter what. But based
on my other comment, the browser timezone not being part of a curated list
should not be a problem if we do not have that hardcoded list.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
