Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

E2e bigquerymultitable #1277

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

E2e bigquerymultitable #1277

wants to merge 3 commits into from

Conversation

itsmekumari
Copy link
Contributor

E2e bigquerymultitable

@Vipinofficial11 Vipinofficial11 added the build Trigger unit test build label Aug 18, 2023
@sau42shri sau42shri requested review from Aryan-Verma and DJSagarAhire and removed request for DJSagarAhire August 22, 2023 08:11
@priyabhatnagar25 priyabhatnagar25 force-pushed the e2e_bigquerymultitable branch 2 times, most recently from 6b14d85 to 638e149 Compare August 22, 2023 10:04
@poojantcs
Copy link

Please incorporate the changes from this PR: data-integrations/database-plugins#418

@priyabhatnagar25 priyabhatnagar25 force-pushed the e2e_bigquerymultitable branch 3 times, most recently from 79d82d2 to cd2286e Compare August 24, 2023 11:46
itsankit-google
itsankit-google reviewed Aug 24, 2023
View reviewed changes
.github/workflows/e2e.yml Outdated
@@ -1,4 +1,4 @@
# Copyright © 2021 Cask Data, Inc.
# Copyright © 2023 Cask Data, Inc.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: 2021-2023

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved

itsankit-google
itsankit-google reviewed Aug 24, 2023
View reviewed changes
src/e2e-test/java/io/cdap/plugin/bigquerymultitable/stepdesign/MultiTableClient.java Outdated
import java.sql.SQLException;
import java.sql.Statement;

import static io.cdap.plugin.bigquerymultitable.stepdesign.BQMultiTableValidation.getMysqlConnection;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: avoid using static imports, it makes the code unreadable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved

itsankit-google
itsankit-google reviewed Aug 24, 2023
View reviewed changes
pom.xml Outdated
Comment on lines 689 to 694
<dependency>
<groupId>mysql</groupId>
<artifactId>mysql-connector-java</artifactId>
<version>8.0.19</version>
<scope>test</scope>
</dependency>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the dependency is only required for e2e-tests, plz add it under e2e-tests profile.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved

itsankit-google
itsankit-google reviewed Aug 24, 2023
View reviewed changes
src/e2e-test/java/io/cdap/plugin/bigquerymultitable/runners/TestRunner.java Outdated
features = {"src/e2e-test/features"},
glue = {"io.cdap.plugin.bigquerymultitable.stepsdesign", "io.cdap.plugin.gcs.stepsdesign", "stepsdesign",
"io.cdap.plugin.common.stepsdesign"},
tags = {"@BQMT_SINK and not @PLUGIN-1669"},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if we can go ahead with the PR before PLUGIN-1669 is implemented.
Almost all the runtime scenarios require multi table plugin and will be skipped.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc: @Vipinofficial11 @poojantcs

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving this PR to draft for now, To run any of the tests which uses BQMT plugin we will have to add a feature to the scripts so we can use plugins from CDAP Hub in the test executions.

itsankit-google
itsankit-google reviewed Aug 24, 2023
View reviewed changes
src/e2e-test/java/io/cdap/plugin/bigquerymultitable/runners/TestRunnerRequired.java Outdated
glue = {"io.cdap.plugin.bigquerymultitable.stepsdesign", "io.cdap.plugin.gcs.stepsdesign", "stepsdesign",
"io.cdap.plugin.common.stepsdesign"},
tags = {"@BQMT_Required"},
//TODO: Enable test once issue is fixed https://cdap.atlassian.net/browse/PLUGIN-1134
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you plz help me understand how is PLUGIN-1134 related?

I cannot see any scenario involving GCSMove plugin in this PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved

itsankit-google
itsankit-google reviewed Aug 24, 2023
View reviewed changes
src/e2e-test/resources/pluginParameters.properties Outdated
sourceTable=dummy
tableNameField=tableName
jdbcPluginName=mysql
connectionString=jdbc:mysql://10.232.0.88/cdfmysqldb
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Plz do not push any Ip Addresses/hostnames in github.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deleted the IP

Aryan-Verma
Aryan-Verma reviewed Aug 27, 2023
View reviewed changes
src/e2e-test/java/io/cdap/plugin/bigquerymultitable/stepdesign/BQMultiTableValidation.java Outdated Show resolved Hide resolved
src/e2e-test/java/io/cdap/plugin/bigquerymultitable/stepdesign/BQMultiTableValidation.java Outdated Show resolved Hide resolved
src/e2e-test/java/io/cdap/plugin/bigquerymultitable/stepdesign/BigQueryMultiTable.java Outdated Show resolved Hide resolved
src/e2e-test/java/io/cdap/plugin/bigquerymultitable/stepdesign/MultiTableClient.java

public static void createSourceDatatypesTable(String sourceTable) throws SQLException, ClassNotFoundException {
try (Connection connect = getMysqlConnection(); Statement statement = connect.createStatement()) {
String createTableQuery = "CREATE TABLE " + sourceTable +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How are we validating multiple tables when we are creating just one table here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As Big Query Multi Table Sink Plugin splits into two Tables. So we have used two existing tables tabA and tabB. Further, extracting the data from both the tables for the validation. This is the source table that is having a column table name which has two values tabA and tabB, which gets split into two table tabA and tabB by BQMT sink plugin.

src/e2e-test/resources/pluginDataCyAttributes.properties Outdated Show resolved Hide resolved
@bharatgulati bharatgulati force-pushed the e2e_bigquerymultitable branch 3 times, most recently from 89e0b0b to f0c2b16 Compare September 1, 2023 05:23
@Vipinofficial11 Vipinofficial11 marked this pull request as draft September 1, 2023 06:18
@bharatgulati bharatgulati added build Trigger unit test build and removed build Trigger unit test build labels Oct 9, 2023
@rahuldash171 rahuldash171 added build Trigger unit test build and removed build Trigger unit test build labels Nov 16, 2023
@priyabhatnagar25 priyabhatnagar25 force-pushed the e2e_bigquerymultitable branch 3 times, most recently from 4c9db2e to 74e2152 Compare November 22, 2023 15:38
@priyabhatnagar25 priyabhatnagar25 force-pushed the e2e_bigquerymultitable branch 8 times, most recently from 170af4a to 568fff4 Compare December 5, 2023 10:43
@rahuldash171 rahuldash171 force-pushed the e2e_bigquerymultitable branch from 91af24c to ae0c3b0 Compare December 20, 2023 09:52
@rahuldash171 rahuldash171 added build Trigger unit test build and removed build Trigger unit test build labels Dec 20, 2023
@rahuldash171 rahuldash171 force-pushed the e2e_bigquerymultitable branch 4 times, most recently from d6ccc26 to d808e6f Compare December 20, 2023 14:27
@rahuldash171 rahuldash171 marked this pull request as ready for review December 21, 2023 08:49
@rahuldash171 rahuldash171 force-pushed the e2e_bigquerymultitable branch from d808e6f to 0d56779 Compare December 21, 2023 08:59
@priyabhatnagar25 @Vipinofficial11
BQMT e2e Tests Scenarios
3579401
@priyabhatnagar25 @Vipinofficial11
Removed Issue tag Plugin-1669 in Runtime scenarios and test runner file,
9740cc8 
updated correct conn string,
removed space in yml,
removed verify goal in pom.
changed sql connector java version to run in github actions and
bqmt secret updated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@priyabhatnagar25 priyabhatnagar25 priyabhatnagar25 left review comments

@Vipinofficial11 Vipinofficial11 Vipinofficial11 left review comments

@itsankit-google itsankit-google itsankit-google left review comments

@Aryan-Verma Aryan-Verma Aryan-Verma left review comments

@DJSagarAhire DJSagarAhire Awaiting requested review from DJSagarAhire

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
build Trigger unit test build
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@itsmekumari @poojantcs @Aryan-Verma @itsankit-google @Vipinofficial11 @priyabhatnagar25 @rahuldash171 @bharatgulati