Elasticsearch: reduce code duplication #5137

mantas-sidlauskas · 2023-03-08T13:58:51Z

What changed?

Removing code duplication form ES clients.
Bulk service related code is moved to a separate file.
TLS and AWS signing clients are initialized in a single place

Why?
To introduce official Elasticsearch client or OpenSearch client, we need to hide client details.
This PR is first of the refactoring series. For easier code review, I will make multiple and small changes.

How did you test it?
Unit tests

Potential risks

Release notes

Documentation Changes

coveralls · 2023-03-08T14:19:24Z

Pull Request Test Coverage Report for Build 0186e662-14e9-4c75-9658-ef067ff0d8a4

142 of 414 (34.3%) changed or added relevant lines in 6 files are covered.
193 unchanged lines in 16 files lost coverage.
Overall coverage decreased (-0.05%) to 57.092%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
common/elasticsearch/interfaces.go	6	17	35.29%
common/elasticsearch/client_v6.go	52	66	78.79%
common/elasticsearch/common.go	0	24	0.0%
common/elasticsearch/client_v6_bulk.go	84	124	67.74%
common/elasticsearch/client_v7.go	0	62	0.0%
common/elasticsearch/client_v7_bulk.go	0	121	0.0%

Files with Coverage Reduction	New Missed Lines	%
service/history/queue/timer_queue_processor_base.go	1	77.26%
common/cache/lru.go	2	92.2%
common/persistence/nosql/nosqlplugin/cassandra/workflow.go	3	59.55%
common/persistence/statsComputer.go	3	94.64%
common/task/fifoTaskScheduler.go	3	84.54%
service/matching/taskListManager.go	3	81.15%
service/history/task/fetcher.go	4	91.24%
common/elasticsearch/client_v6.go	6	61.83%
common/elasticsearch/client_v7.go	9	0%
common/persistence/nosql/nosqlplugin/cassandra/workflowUtils.go	12	75.24%

Totals
Change from base Build 0186e648-d9f3-4b73-a7dd-2543a5c5a7b9:	-0.05%
Covered Lines:	85237
Relevant Lines:	149298

💛 - Coveralls

common/elasticsearch/interfaces.go

Shaddoll · 2023-03-10T19:48:02Z

common/elasticsearch/common.go

+	if err != nil {
+		return nil, err
+	}
+	return esaws.NewV4SigningClient(sess.Config.Credentials, credentialConfig.Region), nil


v6 and v7 are using different esaws packages

they do, but all the code is the same.

Will provide a local signer with a separate PR to remove dependency on olivere es client

common/elasticsearch/client_v7.go

commit f1e2476 Author: sonpham96 <sonpham1996@gmail.com> Date: Sat Mar 18 05:32:01 2023 +0700 Upgrade Golang base image to 1.18 to remediate CVEs (#5035) Co-authored-by: David Porter <david.porter@uber.com> commit 1519ace Author: charlese-instaclustr <76502507+charlese-instaclustr@users.noreply.github.com> Date: Fri Mar 17 22:11:27 2023 +0000 Fix type validation in configstore DC client value updating (#5110) * Remove misleading type check, Add more detailed log message * removing debugging logging * Handle nil update edge case --------- Co-authored-by: allenchen2244 <102192478+allenchen2244@users.noreply.github.com> Co-authored-by: Zijian <Shaddoll@users.noreply.github.com> commit a3e2774 Author: charlese-instaclustr <76502507+charlese-instaclustr@users.noreply.github.com> Date: Fri Mar 17 19:02:40 2023 +0000 Add Canary TLS support (#5086) * add support for TLS connections by Canary, add development config for Canary with TLS * update README to include new config option * remove testing config --------- Co-authored-by: David Porter <david.porter@uber.com> Co-authored-by: Shijie Sheng <shengs@uber.com> Co-authored-by: Zijian <Shaddoll@users.noreply.github.com> commit ff4eab2 Author: Shijie Sheng <shengs@uber.com> Date: Thu Mar 16 20:10:54 2023 -0700 [history] more cautious in deciding domain state to make decisions on dropping queued tasks (#5164) What changed? When domain cache returned entity not found error, don't drop queued tasks to be more conservative. Why? In cases when the cache is dubious, we shouldn't drop the queued tasks. commit 55a8d93 Author: neil-xie <104041627+neil-xie@users.noreply.github.com> Date: Thu Mar 16 14:18:35 2023 -0700 Add Pinot docker files, table config and schema (#5163) * Initial checkin for pinot config files commit 1304570 Author: Mantas Šidlauskas <mantass@netapp.com> Date: Thu Mar 16 15:20:29 2023 +0200 Set poll interval for filebased dynamic config if not set (#5160) * Set poll interval for filebased dynamic config if not set * update unit test commit 42a14b1 Author: Mantas Šidlauskas <mantass@netapp.com> Date: Thu Mar 16 10:49:21 2023 +0200 Elasticsearch: reduce code duplication (#5137) * Elasticsearch: reduce code duplication * address comments --------- Co-authored-by: Zijian <Shaddoll@users.noreply.github.com> commit cbf0d14 Author: bowen xiao <xbowen@uber.com> Date: Wed Mar 15 10:19:34 2023 -0700 fix samples documentation (#5088) commit ba19a29 Author: Mantas Šidlauskas <mantass@netapp.com> Date: Wed Mar 15 12:52:29 2023 +0200 Add ShardID to valid attributes (#5161) commit a25cba8 Author: Mantas Šidlauskas <mantass@netapp.com> Date: Wed Mar 15 10:56:50 2023 +0200 ES: single interface for different ES/OpenSearch versions (#5158) * ES: single interface for different ES/OpenSearch versions * make fmt commit e3ac246 Author: Ketsia <115650494+ketsiambaku@users.noreply.github.com> Date: Tue Mar 14 12:47:40 2023 -0700 added logging with workflow/domain tags (#5159) commit 9581488 Author: Ketsia <115650494+ketsiambaku@users.noreply.github.com> Date: Mon Mar 13 16:56:45 2023 -0700 Consistent query pershard metric (#5143) * added and update consistent query per shard metric * testing pershard metric * move sample logger into persistence metric client for cleaness * fix test * fix lint * fix test again * fix lint * sample logging with workflowid tag * added domain tag to logger * metric completed * addressing comments * fix lint * Revert "fix lint" This reverts commit 1e96944. * fix lint second attempt --------- Co-authored-by: Allen Chen <allenchen2244@uber.com>

commit 9d01035 Author: allenchen2244 <102192478+allenchen2244@users.noreply.github.com> Date: Wed Mar 29 20:50:38 2023 -0700 large workflow hot shard detection (#5166) Metrics for large workflows commit dd51c53 Author: David Porter <david.porter@uber.com> Date: Wed Mar 29 18:30:06 2023 -0700 fix build (#5180) commit 7b281c2 Author: David Porter <david.porter@uber.com> Date: Mon Mar 27 10:38:37 2023 -0700 Adds a small test to catch issues with deadlocks (#5171) * Adds a small test to catch issues with deadlocks commit f1e2476 Author: sonpham96 <sonpham1996@gmail.com> Date: Sat Mar 18 05:32:01 2023 +0700 Upgrade Golang base image to 1.18 to remediate CVEs (#5035) Co-authored-by: David Porter <david.porter@uber.com> commit 1519ace Author: charlese-instaclustr <76502507+charlese-instaclustr@users.noreply.github.com> Date: Fri Mar 17 22:11:27 2023 +0000 Fix type validation in configstore DC client value updating (#5110) * Remove misleading type check, Add more detailed log message * removing debugging logging * Handle nil update edge case --------- Co-authored-by: allenchen2244 <102192478+allenchen2244@users.noreply.github.com> Co-authored-by: Zijian <Shaddoll@users.noreply.github.com> commit a3e2774 Author: charlese-instaclustr <76502507+charlese-instaclustr@users.noreply.github.com> Date: Fri Mar 17 19:02:40 2023 +0000 Add Canary TLS support (#5086) * add support for TLS connections by Canary, add development config for Canary with TLS * update README to include new config option * remove testing config --------- Co-authored-by: David Porter <david.porter@uber.com> Co-authored-by: Shijie Sheng <shengs@uber.com> Co-authored-by: Zijian <Shaddoll@users.noreply.github.com> commit ff4eab2 Author: Shijie Sheng <shengs@uber.com> Date: Thu Mar 16 20:10:54 2023 -0700 [history] more cautious in deciding domain state to make decisions on dropping queued tasks (#5164) What changed? When domain cache returned entity not found error, don't drop queued tasks to be more conservative. Why? In cases when the cache is dubious, we shouldn't drop the queued tasks. commit 55a8d93 Author: neil-xie <104041627+neil-xie@users.noreply.github.com> Date: Thu Mar 16 14:18:35 2023 -0700 Add Pinot docker files, table config and schema (#5163) * Initial checkin for pinot config files commit 1304570 Author: Mantas Šidlauskas <mantass@netapp.com> Date: Thu Mar 16 15:20:29 2023 +0200 Set poll interval for filebased dynamic config if not set (#5160) * Set poll interval for filebased dynamic config if not set * update unit test commit 42a14b1 Author: Mantas Šidlauskas <mantass@netapp.com> Date: Thu Mar 16 10:49:21 2023 +0200 Elasticsearch: reduce code duplication (#5137) * Elasticsearch: reduce code duplication * address comments --------- Co-authored-by: Zijian <Shaddoll@users.noreply.github.com> commit cbf0d14 Author: bowen xiao <xbowen@uber.com> Date: Wed Mar 15 10:19:34 2023 -0700 fix samples documentation (#5088) commit ba19a29 Author: Mantas Šidlauskas <mantass@netapp.com> Date: Wed Mar 15 12:52:29 2023 +0200 Add ShardID to valid attributes (#5161) commit a25cba8 Author: Mantas Šidlauskas <mantass@netapp.com> Date: Wed Mar 15 10:56:50 2023 +0200 ES: single interface for different ES/OpenSearch versions (#5158) * ES: single interface for different ES/OpenSearch versions * make fmt commit e3ac246 Author: Ketsia <115650494+ketsiambaku@users.noreply.github.com> Date: Tue Mar 14 12:47:40 2023 -0700 added logging with workflow/domain tags (#5159) commit 9581488 Author: Ketsia <115650494+ketsiambaku@users.noreply.github.com> Date: Mon Mar 13 16:56:45 2023 -0700 Consistent query pershard metric (#5143) * added and update consistent query per shard metric * testing pershard metric * move sample logger into persistence metric client for cleaness * fix test * fix lint * fix test again * fix lint * sample logging with workflowid tag * added domain tag to logger * metric completed * addressing comments * fix lint * Revert "fix lint" This reverts commit 1e96944. * fix lint second attempt --------- Co-authored-by: Allen Chen <allenchen2244@uber.com>

Shaddoll reviewed Mar 10, 2023

View reviewed changes

common/elasticsearch/interfaces.go Show resolved Hide resolved

Shaddoll reviewed Mar 10, 2023

View reviewed changes

common/elasticsearch/client_v7.go Outdated Show resolved Hide resolved

mantas-sidlauskas force-pushed the esclient_dupl branch 2 times, most recently from 5ece1eb to ff5313f Compare March 13, 2023 18:25

Elasticsearch: reduce code duplication

616b414

mantas-sidlauskas force-pushed the esclient_dupl branch from ff5313f to 616b414 Compare March 14, 2023 09:13

mantas-sidlauskas and others added 4 commits March 14, 2023 12:35

address comments

6288fcb

Merge branch 'master' into esclient_dupl

dcb083b

Merge branch 'master' into esclient_dupl

27ca157

Merge branch 'master' into esclient_dupl

da251e8

Shaddoll approved these changes Mar 15, 2023

View reviewed changes

mindaugasbarcauskas merged commit 42a14b1 into uber:master Mar 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Elasticsearch: reduce code duplication #5137

Elasticsearch: reduce code duplication #5137

mantas-sidlauskas commented Mar 8, 2023

coveralls commented Mar 8, 2023 •

edited

Loading

Shaddoll Mar 10, 2023

mantas-sidlauskas Mar 14, 2023 •

edited

Loading

Elasticsearch: reduce code duplication #5137

Elasticsearch: reduce code duplication #5137

Conversation

mantas-sidlauskas commented Mar 8, 2023

coveralls commented Mar 8, 2023 • edited Loading

Pull Request Test Coverage Report for Build 0186e662-14e9-4c75-9658-ef067ff0d8a4

💛 - Coveralls

Shaddoll Mar 10, 2023

Choose a reason for hiding this comment

mantas-sidlauskas Mar 14, 2023 • edited Loading

Choose a reason for hiding this comment

coveralls commented Mar 8, 2023 •

edited

Loading

mantas-sidlauskas Mar 14, 2023 •

edited

Loading