Read the Pravega ZK and BK configuration option from an input Json file for system tests. #7073

AJadhav29 · 2023-05-02T00:30:43Z

Change log description
Currently, the Pravega ZK and BK configurations for system tests are read from the default config values.

Purpose of the change
Fixes #7072

What the code does
To read the pravega ZK and BK configuration option from an input jSON file for system tests.
Remove the hardcoded values and replace it to accept from json file.

How to verify it
The ZK and BK config values for system tests are read from external input json file when running system tests.

codecov · 2023-05-02T00:56:41Z

Codecov Report

Patch and project coverage have no change.

Comparison is base (ff62002) 86.37% compared to head (cf7f5d4) 86.38%.

❗ Current head cf7f5d4 differs from pull request most recent head 0df8c62. Consider uploading reports for the commit 0df8c62 to get more accurate results

Additional details and impacted files

@@            Coverage Diff            @@
##             master    #7073   +/-   ##
=========================================
  Coverage     86.37%   86.38%           
- Complexity    16084    16085    +1     
=========================================
  Files          1031     1031           
  Lines         59890    59890           
  Branches       6094     6094           
=========================================
+ Hits          51732    51733    +1     
+ Misses         4984     4981    -3     
- Partials       3174     3176    +2

see 16 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

...stem/src/main/java/io/pravega/test/system/framework/services/kubernetes/AbstractService.java

amit-kumar59

Its looks good for me.

...stem/src/main/java/io/pravega/test/system/framework/services/kubernetes/AbstractService.java

anju-c-das · 2023-05-03T08:22:13Z

@AJadhav29 please take care of below points:

Check-in the Json file with required configuration.
Please confirm on the code and include the changes in case the json file is not present, which is default values should be used in such case.
Please mention the build once you run the system tests with new changes.

AJadhav29 · 2023-05-05T03:28:33Z

The system test was green.

...stem/src/main/java/io/pravega/test/system/framework/services/kubernetes/AbstractService.java

anishakj · 2023-05-05T04:56:22Z

@AJadhav29 Could you please verify the values mentioned in the json file are getting applied correctly into the cluster where system test is ran.

test/system/src/test/resources/systemTestConfig.json

...src/main/java/io/pravega/test/system/framework/services/kubernetes/BookkeeperProperties.java

...stem/src/main/java/io/pravega/test/system/framework/services/kubernetes/AbstractService.java

a6dulaleem

please do have a look to the comments @AJadhav29.

a6dulaleem · 2023-05-29T06:44:23Z

...stem/src/main/java/io/pravega/test/system/framework/services/kubernetes/AbstractService.java

-                .put("options", props)
+                .put("controllerResources", getResources(resourceWrapper.getControllerProperties().getControllerResources().getLimits().get("cpu"),
+                        resourceWrapper.getControllerProperties().getControllerResources().getLimits().get("memory"),
+                        resourceWrapper.getControllerProperties().getControllerResources().getRequests().get("cpu"),


Dont pass cpu and memory just call get resources with empty parameter and do all stuff inside the getResources() method because resource Wrapper is present there as well.

I don't think so that's feasible. The getResources() method in the AbstractService class has 4 parameters passed and couldn't be passed empty.

we can refactor it

I don't think refactoring would make any sense here. The only thing we can do then is to remove that method from AbstractService class and pass the value as we did for others.

...stem/src/main/java/io/pravega/test/system/framework/services/kubernetes/AbstractService.java

a6dulaleem · 2023-05-29T06:47:12Z

...src/main/java/io/pravega/test/system/framework/services/kubernetes/BookkeeperProperties.java

+import java.util.Map;
+
+/**
+ * POJO class for bookkeeperProperties.


Could you please give some more context to the comment?

The POJO class itself lists the parameters that it wants to get from the json. I believe that explains the context :)

...src/main/java/io/pravega/test/system/framework/services/kubernetes/ControllerProperties.java

test/system/src/main/java/io/pravega/test/system/framework/services/kubernetes/Probes.java

test/system/src/main/java/io/pravega/test/system/framework/services/kubernetes/Storage.java

.../src/main/java/io/pravega/test/system/framework/services/kubernetes/ZookeeperProperties.java

test/system/src/main/java/io/pravega/test/system/framework/services/kubernetes/Storage.java

...c/main/java/io/pravega/test/system/framework/services/kubernetes/SegmentStoreProperties.java

a6dulaleem

please do have a look to the comments @AJadhav29.

Signed-off-by: AJadhav29 <aditi.jadhav@dell.com> Signed-off-by: AJadhav29 <aditi.jadhav@dell.com>

Signed-off-by: AJadhav29 <aditi.jadhav@dell.com>

Signed-off-by: AJadhav29 <aditi.jadhav@dell.com> Signed-off-by: AJadhav29 <aditi.jadhav@dell.com>

Signed-off-by: AJadhav29 <aditi.jadhav@dell.com>

Signed-off-by: AJadhav29 <aditi.jadhav@dell.com> Signed-off-by: AJadhav29 <aditi.jadhav@dell.com>

Signed-off-by: AJadhav29 <aditi.jadhav@dell.com>

Signed-off-by: AJadhav29 <aditi.jadhav@dell.com> Signed-off-by: AJadhav29 <aditi.jadhav@dell.com>

a6dulaleem

LGTM

abhinb · 2023-06-12T10:58:09Z

...stem/src/main/java/io/pravega/test/system/framework/services/kubernetes/ResourceWrapper.java

+        InputStream stream = ResourceWrapper.class.getClassLoader().getResourceAsStream(configFile);
+        ResourceWrapper resourceWrapper = null;
+        try {
+            resourceWrapper = objectMapper.readValue(stream, ResourceWrapper.class);


we might have to close the mapper or the underlying stream when done, on exception etc?

abhinb · 2023-06-12T10:59:47Z

...stem/src/main/java/io/pravega/test/system/framework/services/kubernetes/AbstractService.java

-    private static final String JOURNALDIRECTORIES = "/bk/journal/j0,/bk/journal/j1,/bk/journal/j2,/bk/journal/j3";
-    private static final String LEDGERDIRECTORIES = "/bk/ledgers/l0,/bk/ledgers/l1,/bk/ledgers/l2,/bk/ledgers/l3";
-
+    private static final String SYSTEMTESTCONFIG = "systemTestConfig.json";


to pass custom config now we edit this file and place it where?

We are placing the json config file in same package in resources folder -
The repository root for the config file is - test/system/src/test/resources/systemTestConfig.json.
But we are passing the sorce root instead for the config file.

abhinb · 2023-06-12T11:00:39Z

...stem/src/main/java/io/pravega/test/system/framework/services/kubernetes/ResourceWrapper.java

+    private ZookeeperProperties zookeeperProperties;
+    private Map<String, String> pravegaOptions;
+
+    public static ResourceWrapper getSystemTestConfig(String configFile) {


can this used be to pass in a custom file path now?

Yes. We are creating a static constant - SYSTEMTESTCONFIG in AbstractService class to pass the config file. Then we are passing that with another declared constant as - ResourceWrapper.getSystemTestConfig(SYSTEMTESTCONFIG).

abhinb · 2023-06-12T11:06:11Z

...stem/src/main/java/io/pravega/test/system/framework/services/kubernetes/AbstractService.java

                        .build())
                .put("requests", ImmutableMap.builder()
-                        .put("cpu", requestsCpu)
-                        .put("memory", requestsMem)
+                        .put("cpu", resources.getRequests().get("cpu"))


can choose to introduce static constants here and at other places.

There are not more than 2 instances of resources.getRequests or resourceWrapper.getControllerProperties or any of the other properties. So, even then do we need to instroduce static constants for each of them?

Signed-off-by: AJadhav29 <aditi.jadhav@dell.com> Signed-off-by: AJadhav29 <aditi.jadhav@dell.com>

tkaitchuck · 2023-09-14T19:23:50Z

...src/main/java/io/pravega/test/system/framework/services/kubernetes/BookkeeperProperties.java

+ * POJO class for bookkeeperProperties.
+ */
+@Getter
+@Setter


Please use lombok + Jackson annotations to make these immutable
https://www.baeldung.com/jackson-deserialize-immutable-objects
https://projectlombok.org/features/Builder

tkaitchuck · 2023-09-14T19:23:59Z

...src/main/java/io/pravega/test/system/framework/services/kubernetes/ControllerProperties.java

+ * POJO class for controllerProperties.
+ */
+@Getter
+@Setter


tkaitchuck · 2023-09-14T19:26:16Z

test/system/src/main/java/io/pravega/test/system/framework/services/kubernetes/Probes.java

+ * POJO class for bookkeeper probes.
+ */
+@Getter
+@Setter


tkaitchuck · 2023-09-14T19:26:25Z

...stem/src/main/java/io/pravega/test/system/framework/services/kubernetes/ResourceWrapper.java

+ */
+@Slf4j
+@Getter
+@Setter


tkaitchuck · 2023-09-14T19:26:35Z

test/system/src/main/java/io/pravega/test/system/framework/services/kubernetes/Resources.java

+ * POJO class for getting resources for controller, segmentStore, zookeeper and bookkeeper.
+ */
+@Getter
+@Setter


tkaitchuck · 2023-09-14T19:26:43Z

...c/main/java/io/pravega/test/system/framework/services/kubernetes/SegmentStoreProperties.java

+ * POJO class for segmentStoreProperties.
+ */
+@Getter
+@Setter


tkaitchuck · 2023-09-14T19:26:49Z

test/system/src/main/java/io/pravega/test/system/framework/services/kubernetes/Storage.java

+ * POJO class for getting storage values for bookkeeperStorage.
+ */
+@Getter
+@Setter


AJadhav29 marked this pull request as draft May 2, 2023 00:52

AJadhav29 force-pushed the karman-3462-zkbkConfig branch 3 times, most recently from 96f7382 to fa89ff7 Compare May 2, 2023 07:24

AJadhav29 marked this pull request as ready for review May 2, 2023 07:46

AJadhav29 force-pushed the karman-3462-zkbkConfig branch from 3fb7761 to fa89ff7 Compare May 2, 2023 08:32

anju-c-das reviewed May 2, 2023

View reviewed changes

...stem/src/main/java/io/pravega/test/system/framework/services/kubernetes/AbstractService.java Show resolved Hide resolved

amit-kumar59 approved these changes May 2, 2023

View reviewed changes

anju-c-das reviewed May 2, 2023

View reviewed changes

...stem/src/main/java/io/pravega/test/system/framework/services/kubernetes/AbstractService.java Outdated Show resolved Hide resolved

AnkurAnand11 reviewed May 2, 2023

View reviewed changes

...stem/src/main/java/io/pravega/test/system/framework/services/kubernetes/AbstractService.java Outdated Show resolved Hide resolved

AJadhav29 force-pushed the karman-3462-zkbkConfig branch from d17ec52 to d9e200a Compare May 2, 2023 23:14

anishakj requested a review from abhinb May 3, 2023 16:09

anju-c-das reviewed May 5, 2023

View reviewed changes

...stem/src/main/java/io/pravega/test/system/framework/services/kubernetes/AbstractService.java Outdated Show resolved Hide resolved

anju-c-das reviewed May 5, 2023

View reviewed changes

...stem/src/main/java/io/pravega/test/system/framework/services/kubernetes/AbstractService.java Outdated Show resolved Hide resolved

anishakj reviewed May 5, 2023

View reviewed changes

test/system/src/test/resources/systemTestConfig.json Outdated Show resolved Hide resolved

AJadhav29 marked this pull request as draft May 5, 2023 07:02

anju-c-das reviewed May 11, 2023

View reviewed changes

...src/main/java/io/pravega/test/system/framework/services/kubernetes/BookkeeperProperties.java Outdated Show resolved Hide resolved

AJadhav29 force-pushed the karman-3462-zkbkConfig branch from 40428d9 to c16a803 Compare May 16, 2023 04:10

AJadhav29 marked this pull request as ready for review May 23, 2023 02:19

anishakj reviewed May 23, 2023

View reviewed changes

...stem/src/main/java/io/pravega/test/system/framework/services/kubernetes/AbstractService.java Outdated Show resolved Hide resolved

AJadhav29 marked this pull request as draft May 24, 2023 03:23

AJadhav29 marked this pull request as ready for review May 25, 2023 22:57

AJadhav29 force-pushed the karman-3462-zkbkConfig branch from 6e92afa to d0e78f5 Compare May 26, 2023 04:25

a6dulaleem reviewed May 29, 2023

View reviewed changes

AJadhav29 force-pushed the karman-3462-zkbkConfig branch from 9e436e0 to f3f8da7 Compare June 5, 2023 05:37

AJadhav29 added 25 commits June 4, 2023 23:48

Changing the path of json file.

9ffe82b

Signed-off-by: AJadhav29 <aditi.jadhav@dell.com> Signed-off-by: AJadhav29 <aditi.jadhav@dell.com>

Working on deserialization to read the json input file.

b765ecd

Signed-off-by: AJadhav29 <aditi.jadhav@dell.com> Signed-off-by: AJadhav29 <aditi.jadhav@dell.com>

Changing the path for json input file.

ea84e68

Signed-off-by: AJadhav29 <aditi.jadhav@dell.com> Signed-off-by: AJadhav29 <aditi.jadhav@dell.com>

Adding the log.info and changing the path for json input file.

0a5b7cc

Signed-off-by: AJadhav29 <aditi.jadhav@dell.com> Signed-off-by: AJadhav29 <aditi.jadhav@dell.com>

changing getSystemTestConfig

392b6ec

Signed-off-by: AJadhav29 <aditi.jadhav@dell.com>

adding new class JSONReader

1daf53a

Signed-off-by: AJadhav29 <aditi.jadhav@dell.com>

Modifying JSONReader

beae053

Signed-off-by: AJadhav29 <aditi.jadhav@dell.com>

Modifying ResourceWrapper

010482a

Signed-off-by: AJadhav29 <aditi.jadhav@dell.com>

Modifying ResourceWrapper

ac76b8d

Signed-off-by: AJadhav29 <aditi.jadhav@dell.com>

Addressing review comments and modifying the json.

27aa2fe

Signed-off-by: AJadhav29 <aditi.jadhav@dell.com> Signed-off-by: AJadhav29 <aditi.jadhav@dell.com>

Addressing review comment.

6c446c6

Signed-off-by: AJadhav29 <aditi.jadhav@dell.com> Signed-off-by: AJadhav29 <aditi.jadhav@dell.com>

Adding probes to AbstractService.

ab1b3d3

Signed-off-by: AJadhav29 <aditi.jadhav@dell.com> Signed-off-by: AJadhav29 <aditi.jadhav@dell.com>

Making changes to AbstractService.

ef6d543

Signed-off-by: AJadhav29 <aditi.jadhav@dell.com> Signed-off-by: AJadhav29 <aditi.jadhav@dell.com>

Making changes to probes in AbstractService.

0a69961

Signed-off-by: AJadhav29 <aditi.jadhav@dell.com> Signed-off-by: AJadhav29 <aditi.jadhav@dell.com>

Making changes to probes in AbstractService.

f9eaaa9

Signed-off-by: AJadhav29 <aditi.jadhav@dell.com> Signed-off-by: AJadhav29 <aditi.jadhav@dell.com>

Reconfiguring the json file.

710a8bf

Signed-off-by: AJadhav29 <aditi.jadhav@dell.com> Signed-off-by: AJadhav29 <aditi.jadhav@dell.com>

Adding comments to the POJO classes.

2a1c57f

Signed-off-by: AJadhav29 <aditi.jadhav@dell.com> Signed-off-by: AJadhav29 <aditi.jadhav@dell.com>

making changes to options

620a48c

Signed-off-by: AJadhav29 <aditi.jadhav@dell.com>

making changes to pravegaDeployment

7166669

Signed-off-by: AJadhav29 <aditi.jadhav@dell.com>

Removing log statements from ResourceWrapper class.

a2d33d1

Signed-off-by: AJadhav29 <aditi.jadhav@dell.com> Signed-off-by: AJadhav29 <aditi.jadhav@dell.com>

making changes to json

69d59b9

Signed-off-by: AJadhav29 <aditi.jadhav@dell.com>

Adding getCombinedMap method.

1ec0371

Signed-off-by: AJadhav29 <aditi.jadhav@dell.com> Signed-off-by: AJadhav29 <aditi.jadhav@dell.com>

Removing the log line.

4d444cb

Signed-off-by: AJadhav29 <aditi.jadhav@dell.com> Signed-off-by: AJadhav29 <aditi.jadhav@dell.com>

Addressing review comments.

23c9f91

Signed-off-by: AJadhav29 <aditi.jadhav@dell.com> Signed-off-by: AJadhav29 <aditi.jadhav@dell.com>

Refactoring getResources method.

b75dbf6

Signed-off-by: AJadhav29 <aditi.jadhav@dell.com> Signed-off-by: AJadhav29 <aditi.jadhav@dell.com>

AJadhav29 force-pushed the karman-3462-zkbkConfig branch from f3f8da7 to b75dbf6 Compare June 5, 2023 07:07

a6dulaleem approved these changes Jun 5, 2023

View reviewed changes

abhinb reviewed Jun 12, 2023

View reviewed changes

Addressing review comment.

0df8c62

Signed-off-by: AJadhav29 <aditi.jadhav@dell.com> Signed-off-by: AJadhav29 <aditi.jadhav@dell.com>

tkaitchuck reviewed Sep 14, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Read the Pravega ZK and BK configuration option from an input Json file for system tests. #7073

Read the Pravega ZK and BK configuration option from an input Json file for system tests. #7073

AJadhav29 commented May 2, 2023

codecov bot commented May 2, 2023 •

edited

amit-kumar59 left a comment

anju-c-das commented May 3, 2023

AJadhav29 commented May 5, 2023 •

edited

anishakj commented May 5, 2023

a6dulaleem left a comment

a6dulaleem May 29, 2023

AJadhav29 May 31, 2023 •

edited

a6dulaleem Jun 1, 2023

AJadhav29 Jun 1, 2023

a6dulaleem May 29, 2023

AJadhav29 May 31, 2023

a6dulaleem left a comment

a6dulaleem left a comment

abhinb Jun 12, 2023

abhinb Jun 12, 2023

AJadhav29 Jun 13, 2023 •

edited

abhinb Jun 12, 2023

AJadhav29 Jun 13, 2023

abhinb Jun 12, 2023

AJadhav29 Jun 16, 2023

tkaitchuck Sep 14, 2023

tkaitchuck Sep 14, 2023

tkaitchuck Sep 14, 2023

tkaitchuck Sep 14, 2023

tkaitchuck Sep 14, 2023

tkaitchuck Sep 14, 2023

tkaitchuck Sep 14, 2023

Read the Pravega ZK and BK configuration option from an input Json file for system tests. #7073

Are you sure you want to change the base?

Read the Pravega ZK and BK configuration option from an input Json file for system tests. #7073

Conversation

AJadhav29 commented May 2, 2023

codecov bot commented May 2, 2023 • edited

Codecov Report

amit-kumar59 left a comment

Choose a reason for hiding this comment

anju-c-das commented May 3, 2023

AJadhav29 commented May 5, 2023 • edited

anishakj commented May 5, 2023

a6dulaleem left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AJadhav29 May 31, 2023 • edited

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

a6dulaleem left a comment

Choose a reason for hiding this comment

a6dulaleem left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AJadhav29 Jun 13, 2023 • edited

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

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

codecov bot commented May 2, 2023 •

edited

AJadhav29 commented May 5, 2023 •

edited

AJadhav29 May 31, 2023 •

edited

AJadhav29 Jun 13, 2023 •

edited