ETag による通信量削減 #199 の解決 #233

i-maruyama · 2021-06-12T23:03:40Z

Issue 番号 / Issue ID

Close ETag による通信量削減 #199

目的 / Purpose

ETag による通信量削減 ETag による通信量削減 #199 の解決

破壊的変更をもたらしますか / Does this introduce a breaking change?

[x] Yes
[ ] No

Pull Request の種類 / Pull Request type

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Documentation content changes
[ ] Other... Please describe:

検証方法 / How to test

コードの入手 / Get the code

git clone [repo-address]
cd [repo-name]
git checkout [branch-name]
dotnet restore

コードの検証 / Test the code

確認事項 / What to check

iOS での確認

Android エミュレーター、Android 実機では確認しました。

その他 / Other information

Internal IDs:

NFR 2527

b-wind

自分の思う疑問点＋改善案をコメントしました。
他の人が別の案を出されている場合はスルーして貰って結構です。

b-wind · 2021-06-13T00:34:58Z

Covid19Radar/Covid19Radar/Common/PreferenceKey.cs

@@ -9,6 +9,7 @@ public static class PreferenceKey
        // for preferences
        public static string StartDateTime = "StartDateTime";
        public static string LastProcessTekTimestamp = "LastProcessTekTimestamp";
+        public static string ETag = "Etag";


なんの通信での Etag なのか分からなくなりますね。他のところで使いたくなると名前が衝突してしまいますし。
長いですが、 LastProcessTekEtag 等の方が良いのではないかと思います。

取り込みます。

b-wind · 2021-06-13T00:37:22Z

Covid19Radar/Covid19Radar/Services/ExposureNotificationHandler.cs

@@ -170,7 +170,9 @@ public async Task FetchExposureKeyBatchFilesFromServerAsync(Func<IEnumerable<str
                    await submitBatches(downloadedFiles);

                    exposureNotificationService.SetLastProcessTekTimestamp(serverRegion, newCreated);
-                    loggerService.Info($"region: {serverRegion}, lastCreated: {newCreated}");
+                    var etag = Xamarin.Essentials.Preferences.Get("ETag", "");


個人的にはここでHTTP通信である依存を入れたくないので他のクラスでEtagの値を引き回さない方が良いと思います。

個人的な案としては、HttpDataService のところでコメントします。

b-wind · 2021-06-13T00:45:48Z

Covid19Radar/Covid19Radar/Services/HttpDataService.cs

+            var etags = headers.GetValues("ETag");
+            etag = etags.First();


一応 responce に Etag Header が存在しないケースもカバーして置いた方がよりよいかと。

b-wind · 2021-06-13T00:50:26Z

Covid19Radar/Covid19Radar/Services/HttpDataService.cs

            {
                return await result.Content.ReadAsStringAsync();
            }
+            else if (status == System.Net.HttpStatusCode.NotModified)
+            {
+                return "[]";


ごく個人的な意見ですが、ここで前回の Content と同じデータを返してしまえば HTTP のレイヤーで変更範囲を封じ込めることが出来そうです。
Content を保存する場所が必要ですが、数kbyte 程度なので影響は少ないかなと。
流石に preferences に保存するのは気が引けますが。

この後の関数の部分で、中身がなければ、次の処理に進まずに終了するルーチンがあります。処理を軽くするためには、この方が良いです。

cocoa/Covid19Radar/Covid19Radar/Services/ExposureNotificationHandler.cs

Lines 227 to 233 in c29d7a7

List<TemporaryExposureKeyExportFileModel> tekList = await httpDataService.GetTemporaryExposureKeyList(region, cancellationToken);

if (tekList.Count == 0)

{

loggerService.EndMethod();

return (-1, downloadedFiles);

}

Debug.WriteLine("C19R Fetch Exposure Key");

b-wind · 2021-06-13T00:50:41Z

Covid19Radar/Covid19Radar/Services/ExposureNotificationService.cs

+            SetKey<string>(region, PreferenceKey.ETag, etag);
+        }
+
+        public void SetKey<T>(string region, string key, T created)


Etag の値を Dictionary で保存するのは少々大げさな気がしますね。
メソッドを共通化するほどでは無いかなと。

プログラム的な重要さは、 LastProcessTektimestamp と同じと考えますので、同じ処理にしたいと考えています。

ETagによって、TekList をダウンロードするか決めます。

LastProcessTektimestamp によって、各 TeK zip file をダウンロードするか決めます。

すべての処理が終われば、両方更新します。

b-wind · 2021-06-13T00:55:40Z

Covid19Radar/Covid19Radar/Services/ExposureNotificationHandler.cs

@@ -224,6 +226,7 @@ private async Task<(long, List<string>)> DownloadBatchAsync(string region, long

            var httpDataService = HttpDataService;

+            Xamarin.Essentials.Preferences.Set("ETag", ExposureNotificationService.GetETag(region));


ExposureNotificationService （の中の PreferenceService）と Xamarin.Essentioals.Preferences の両方に Etag を持つ意味は何かありますか？

ETagによって、TekList をダウンロードするか決めます。

LastProcessTektimestamp によって、各 TeK zip file をダウンロードするか決めます。

すべての処理が終われば、両方更新します。

上に述べたように、　ETag の更新は最後に行うため、情報を最後まで保持する必要があります。そのための、Xamarin.Essentioals.Preferences です。

本当は、関数の引数の中と局所変数の中に ETag 情報を持ちたいのですが、変更する部分が大きくなって、うまい案が思いつかなかったというところで、ここはもう少し良案があればXamarin.Essentioals.Preferences から脱却したいところです。

b-wind · 2021-06-13T01:13:14Z

このPRを見たときからコンテンツキャッシュを持っておくと処理が楽そうと思っていてちょうど良さそうなモノを見つけました。
https://github.com/jamesmontemagno/monkey-cache

依存も少ないし、使い方もシンプルなのでどうかなと。

b-wind · 2021-06-13T01:33:56Z

submitBatches の前後で転けた場合、全く同じデータをダウンロードして再処理する必要がある（ #17 ）ので HttpDataService でキャッシュするのは必須かも？

i-maruyama · 2021-06-13T02:45:07Z

submitBatches の前後で転けた場合、全く同じデータをダウンロードして再処理する必要がある（ #17 ）ので HttpDataService でキャッシュするのは必須かも？

こういう枠組みもあり得ますね。ただ、iOS では「何度も登録させられる問題」があるらしいですが、この辺りは、データ容量ぎりぎりの時に保存データが勝手に消去されているという噂（＝SNSで見た未確認情報）があり、この噂を信じるなら、データは少ない方が良いなら、 ETag 情報のみが良いかもしれません。

b-wind · 2021-06-13T02:54:29Z

#17 のケースでは最後まで処理が進まない（ LastProcessTektimestamp も更新されない）ので再度処理を行う必要があります。
この際 list.json が更新されていれば良いですが、 list.json が更新されておらず Etag の一致によって 302 応答が返ってきた場合、場合 HttpDataService が空配列を返すと、その時点追加されている Tek の処理がスキップされてしまいます。

再度 list.json が更新されたときにその分はチェックされるので、トータルで抜けは無い筈ですがたまたま該当者だった場合通知が遅れることになります。
これはCOCOAとしては許容出来ない部分に思えますので、何らかの手当は必要かと。

i-maruyama · 2021-06-13T04:23:41Z

この際 list.json が更新されていれば良いですが、 list.json が更新されておらず Etag の一致によって 302 応答が返ってきた場合、

これは、現在でも起こらないはずなのですが、ご確認ください。 LastProcessTektimestamp と同時に ETag が更新されますので、接触確認に失敗すれば list.json は再度読み込まれます。

b-wind · 2021-06-13T05:45:41Z

失礼しまいした。確かに連続した行で setEtag してますね。（自分のレビューコメントに紛れて見えてませんでした）

当座の懸念は払拭されたと言う事であれば、後は Xamarin.Essentials.Preferences ですかねぇ。
気のせいで無ければ、 Android だとGoogleのバックアップ対象にもなっているはずでちょっと嬉しくないです。
全部のメソッドに引数追加するのもどうかと思いますし、この辺は自分ももう少し手が無いか考えてみます。

i-maruyama added 3 commits May 29, 2021 08:19

show case for issue cocoa-mhlw#199

32c1c57

avoid error-log with the code: return "[]"

d21e8cf

Merge branch 'cocoa-mhlw:develop' into issue#199

d5c0b2f

i-maruyama changed the title ~~Issue#199~~ ETag による通信量削減 #199 の解決 Jun 12, 2021

b-wind reviewed Jun 13, 2021

View reviewed changes

cocoa-dev added the confirmed 開発内部管理用 label Jun 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ETag による通信量削減 #199 の解決 #233

ETag による通信量削減 #199 の解決 #233

i-maruyama commented Jun 12, 2021 •

edited by cocoa-dev

Loading

b-wind left a comment

b-wind Jun 13, 2021

i-maruyama Jun 13, 2021

b-wind Jun 13, 2021

b-wind Jun 13, 2021

b-wind Jun 13, 2021

i-maruyama Jun 13, 2021 •

edited

Loading

b-wind Jun 13, 2021

i-maruyama Jun 13, 2021

b-wind Jun 13, 2021

i-maruyama Jun 13, 2021

b-wind commented Jun 13, 2021

b-wind commented Jun 13, 2021

i-maruyama commented Jun 13, 2021

b-wind commented Jun 13, 2021 •

edited

Loading

i-maruyama commented Jun 13, 2021

b-wind commented Jun 13, 2021

	List<TemporaryExposureKeyExportFileModel> tekList = await httpDataService.GetTemporaryExposureKeyList(region, cancellationToken);
	if (tekList.Count == 0)
	{
	loggerService.EndMethod();
	return (-1, downloadedFiles);
	}
	Debug.WriteLine("C19R Fetch Exposure Key");

		@@ -224,6 +226,7 @@ private async Task<(long, List<string>)> DownloadBatchAsync(string region, long

		var httpDataService = HttpDataService;

		Xamarin.Essentials.Preferences.Set("ETag", ExposureNotificationService.GetETag(region));

ETag による通信量削減 #199 の解決 #233

Are you sure you want to change the base?

ETag による通信量削減 #199 の解決 #233

Conversation

i-maruyama commented Jun 12, 2021 • edited by cocoa-dev Loading

Issue 番号 / Issue ID

目的 / Purpose

破壊的変更をもたらしますか / Does this introduce a breaking change?

Pull Request の種類 / Pull Request type

検証方法 / How to test

コードの入手 / Get the code

コードの検証 / Test the code

確認事項 / What to check

その他 / Other information

b-wind left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

i-maruyama Jun 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b-wind commented Jun 13, 2021

b-wind commented Jun 13, 2021

i-maruyama commented Jun 13, 2021

b-wind commented Jun 13, 2021 • edited Loading

i-maruyama commented Jun 13, 2021

b-wind commented Jun 13, 2021

i-maruyama commented Jun 12, 2021 •

edited by cocoa-dev

Loading

i-maruyama Jun 13, 2021 •

edited

Loading

b-wind commented Jun 13, 2021 •

edited

Loading