You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@gobblin.apache.org by GitBox <gi...@apache.org> on 2021/06/04 23:10:53 UTC

[GitHub] [gobblin] arjun4084346 opened a new pull request #3298: correct column length in MysqlSpecStore, MysqlDagStore, MysqlJobStatu…

arjun4084346 opened a new pull request #3298:
URL: https://github.com/apache/gobblin/pull/3298


   …sStore
   
   Dear Gobblin maintainers,
   
   Please accept this PR. I understand that it will not be reviewed until I have checked off all the steps below!
   
   
   ### JIRA
   - [ ] My PR addresses the following [Gobblin JIRA](https://issues.apache.org/jira/browse/GOBBLIN/) issues and references them in the PR title. For example, "[GOBBLIN-XXX] My Gobblin PR"
       - https://issues.apache.org/jira/browse/GOBBLIN-XXX
   
   
   ### Description
   - [ ] Here are some details about my PR, including screenshots (if applicable):
   
   
   ### Tests
   - [ ] My PR adds the following unit tests __OR__ does not need testing for this extremely good reason:
   
   
   ### Commits
   - [ ] My commits all reference JIRA issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
       1. Subject is separated from body by a blank line
       2. Subject is limited to 50 characters
       3. Subject does not end with a period
       4. Subject uses the imperative mood ("add", not "adding")
       5. Body wraps at 72 characters
       6. Body explains "what" and "why", not "how"
   
   


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [gobblin] aplex commented on a change in pull request #3298: [GOBBLIN-1459] correct column length in MysqlSpecStore, MysqlDagStore, MysqlJobStatu…

Posted by GitBox <gi...@apache.org>.
aplex commented on a change in pull request #3298:
URL: https://github.com/apache/gobblin/pull/3298#discussion_r651940989



##########
File path: gobblin-metastore/src/main/java/org/apache/gobblin/MysqlDagStore.java
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.gobblin;
+
+import java.io.IOException;
+
+import javax.sql.DataSource;
+import lombok.extern.slf4j.Slf4j;
+
+import org.apache.gobblin.configuration.State;
+import org.apache.gobblin.metastore.MysqlStateStore;
+import org.apache.gobblin.service.ServiceConfigKeys;
+
+
+@Slf4j
+/**
+ * An implementation of {@link MysqlStateStore} backed by MySQL to store Dag.
+ *
+ * @param <T> state object type
+ **/
+public class MysqlDagStore<T extends State> extends MysqlStateStore<T> {
+  /**
+   * Manages the persistence and retrieval of {@link State} in a MySQL database
+   * @param dataSource the {@link DataSource} object for connecting to MySQL
+   * @param stateStoreTableName the table for storing the state in rows keyed by two levels (store_name, table_name)
+   * @param compressedValues should values be compressed for storage?
+   * @param stateClass class of the {@link State}s stored in this state store
+   * @throws IOException in case of failures
+   */
+  public MysqlDagStore(DataSource dataSource, String stateStoreTableName, boolean compressedValues,
+      Class<T> stateClass)
+      throws IOException {
+    super(dataSource, stateStoreTableName, compressedValues, stateClass);
+  }
+
+  @Override
+  protected String getCreateJobStateTableTemplate() {
+    int maxStoreName = ServiceConfigKeys.MAX_FLOW_NAME + ServiceConfigKeys.STATE_STORE_KEY_SEPARATION_CHARACTER.length()
+        + ServiceConfigKeys.MAX_FLOW_GROUP;
+    int maxTableName = 13; // length of flowExecutionId which is epoch timestamp
+
+    return "CREATE TABLE IF NOT EXISTS $TABLE$ (store_name varchar(" + maxStoreName + ") CHARACTER SET latin1 COLLATE latin1_bin not null,"

Review comment:
       ok, what would the the actual value they need to set? Looks like its is computed




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [gobblin] aplex commented on a change in pull request #3298: [GOBBLIN-1459] correct column length in MysqlSpecStore, MysqlDagStore, MysqlJobStatu…

Posted by GitBox <gi...@apache.org>.
aplex commented on a change in pull request #3298:
URL: https://github.com/apache/gobblin/pull/3298#discussion_r651970380



##########
File path: gobblin-metastore/src/main/java/org/apache/gobblin/metastore/MysqlJobStatusStateStore.java
##########
@@ -70,6 +71,21 @@ public MysqlJobStatusStateStore(DataSource dataSource, String stateStoreTableNam
     return getAll(storeName, flowExecutionId + "%", true);
   }
 
+  @Override
+  protected String getCreateJobStateTableTemplate() {

Review comment:
       The test can write the state for the flow with max length, and max expected table length, and then try to read it.




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [gobblin] arjun4084346 commented on a change in pull request #3298: [GOBBLIN-1459] correct column length in MysqlSpecStore, MysqlDagStore, MysqlJobStatu…

Posted by GitBox <gi...@apache.org>.
arjun4084346 commented on a change in pull request #3298:
URL: https://github.com/apache/gobblin/pull/3298#discussion_r652023509



##########
File path: gobblin-api/src/main/java/org/apache/gobblin/service/ServiceConfigKeys.java
##########
@@ -132,4 +132,13 @@
   // Group Membership authentication service
   public static final String GROUP_OWNERSHIP_SERVICE_CLASS = GOBBLIN_SERVICE_PREFIX + "groupOwnershipService.class";
   public static final String DEFAULT_GROUP_OWNERSHIP_SERVICE = "org.apache.gobblin.service.NoopGroupOwnershipService";
+
+  public static final int MAX_FLOW_NAME_LENGTH = 128; // defined in FlowId.pdl
+  public static final int MAX_FLOW_GROUP_LENGTH = 128; // defined in FlowId.pdl
+  public static final int MAX_JOB_NAME_LENGTH = 374;

Review comment:
       @aplex @sv2000 will add a check in compiler to not produce job names more than this limit.




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [gobblin] arjun4084346 commented on a change in pull request #3298: [GOBBLIN-1459] correct column length in MysqlSpecStore, MysqlDagStore, MysqlJobStatu…

Posted by GitBox <gi...@apache.org>.
arjun4084346 commented on a change in pull request #3298:
URL: https://github.com/apache/gobblin/pull/3298#discussion_r651970452



##########
File path: gobblin-metastore/src/main/java/org/apache/gobblin/MysqlDagStore.java
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.gobblin;
+
+import java.io.IOException;
+
+import javax.sql.DataSource;
+import lombok.extern.slf4j.Slf4j;
+
+import org.apache.gobblin.configuration.State;
+import org.apache.gobblin.metastore.MysqlStateStore;
+import org.apache.gobblin.service.ServiceConfigKeys;
+
+
+@Slf4j
+/**
+ * An implementation of {@link MysqlStateStore} backed by MySQL to store Dag.
+ *
+ * @param <T> state object type
+ **/
+public class MysqlDagStore<T extends State> extends MysqlStateStore<T> {

Review comment:
       Yes, we have job_status store and flow spec store besides dag store.
   job_status store is also a MysqlStateStore like this dag store. Spec store is different in its own.
   So I have created a new method getCreateJobStateTableTemplate that new stores should override to enforce their own limits.




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [gobblin] arjun4084346 commented on pull request #3298: [GOBBLIN-1459] correct column length in MysqlSpecStore, MysqlDagStore, MysqlJobStatu…

Posted by GitBox <gi...@apache.org>.
arjun4084346 commented on pull request #3298:
URL: https://github.com/apache/gobblin/pull/3298#issuecomment-870890131


   @sv2000  please review/merge


-- 
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: dev-unsubscribe@gobblin.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [gobblin] arjun4084346 commented on a change in pull request #3298: [GOBBLIN-1459] correct column length in MysqlSpecStore, MysqlDagStore, MysqlJobStatu…

Posted by GitBox <gi...@apache.org>.
arjun4084346 commented on a change in pull request #3298:
URL: https://github.com/apache/gobblin/pull/3298#discussion_r651971057



##########
File path: gobblin-metastore/src/main/java/org/apache/gobblin/MysqlDagStore.java
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.gobblin;
+
+import java.io.IOException;
+
+import javax.sql.DataSource;
+import lombok.extern.slf4j.Slf4j;
+
+import org.apache.gobblin.configuration.State;
+import org.apache.gobblin.metastore.MysqlStateStore;
+import org.apache.gobblin.service.ServiceConfigKeys;
+
+
+@Slf4j
+/**
+ * An implementation of {@link MysqlStateStore} backed by MySQL to store Dag.
+ *
+ * @param <T> state object type
+ **/
+public class MysqlDagStore<T extends State> extends MysqlStateStore<T> {
+  /**
+   * Manages the persistence and retrieval of {@link State} in a MySQL database
+   * @param dataSource the {@link DataSource} object for connecting to MySQL
+   * @param stateStoreTableName the table for storing the state in rows keyed by two levels (store_name, table_name)
+   * @param compressedValues should values be compressed for storage?
+   * @param stateClass class of the {@link State}s stored in this state store
+   * @throws IOException in case of failures
+   */
+  public MysqlDagStore(DataSource dataSource, String stateStoreTableName, boolean compressedValues,
+      Class<T> stateClass)
+      throws IOException {
+    super(dataSource, stateStoreTableName, compressedValues, stateClass);
+  }
+
+  @Override
+  protected String getCreateJobStateTableTemplate() {
+    int maxStoreName = ServiceConfigKeys.MAX_FLOW_NAME + ServiceConfigKeys.STATE_STORE_KEY_SEPARATION_CHARACTER.length()
+        + ServiceConfigKeys.MAX_FLOW_GROUP;
+    int maxTableName = 13; // length of flowExecutionId which is epoch timestamp
+
+    return "CREATE TABLE IF NOT EXISTS $TABLE$ (store_name varchar(" + maxStoreName + ") CHARACTER SET latin1 COLLATE latin1_bin not null,"

Review comment:
       Yes, it is computed. I will work with SREs to do a one time manual update.




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [gobblin] arjun4084346 commented on a change in pull request #3298: [GOBBLIN-1459] correct column length in MysqlSpecStore, MysqlDagStore, MysqlJobStatu…

Posted by GitBox <gi...@apache.org>.
arjun4084346 commented on a change in pull request #3298:
URL: https://github.com/apache/gobblin/pull/3298#discussion_r652023509



##########
File path: gobblin-api/src/main/java/org/apache/gobblin/service/ServiceConfigKeys.java
##########
@@ -132,4 +132,13 @@
   // Group Membership authentication service
   public static final String GROUP_OWNERSHIP_SERVICE_CLASS = GOBBLIN_SERVICE_PREFIX + "groupOwnershipService.class";
   public static final String DEFAULT_GROUP_OWNERSHIP_SERVICE = "org.apache.gobblin.service.NoopGroupOwnershipService";
+
+  public static final int MAX_FLOW_NAME_LENGTH = 128; // defined in FlowId.pdl
+  public static final int MAX_FLOW_GROUP_LENGTH = 128; // defined in FlowId.pdl
+  public static final int MAX_JOB_NAME_LENGTH = 374;

Review comment:
       @aplex @sv2000, I will add a check in compiler to not produce job names more than this limit in the separate PR.




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [gobblin] codecov-commenter edited a comment on pull request #3298: [GOBBLIN-1459] correct column length in MysqlSpecStore, MysqlDagStore, MysqlJobStatu…

Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #3298:
URL: https://github.com/apache/gobblin/pull/3298#issuecomment-855118164


   # [Codecov](https://codecov.io/gh/apache/gobblin/pull/3298?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#3298](https://codecov.io/gh/apache/gobblin/pull/3298?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6faa154) into [master](https://codecov.io/gh/apache/gobblin/commit/75f6afff6a39c01569c63ac7895ea692610fbd2c?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (75f6aff) will **decrease** coverage by `0.01%`.
   > The diff coverage is `30.30%`.
   
   > :exclamation: Current head 6faa154 differs from pull request most recent head 92f183e. Consider uploading reports for the commit 92f183e to get more accurate results
   [![Impacted file tree graph](https://codecov.io/gh/apache/gobblin/pull/3298/graphs/tree.svg?width=650&height=150&src=pr&token=4MgURJ0bGc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/gobblin/pull/3298?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #3298      +/-   ##
   ============================================
   - Coverage     46.47%   46.46%   -0.02%     
     Complexity    10023    10023              
   ============================================
     Files          2037     2039       +2     
     Lines         79294    79319      +25     
     Branches       8843     8840       -3     
   ============================================
   + Hits          36852    36853       +1     
   - Misses        39020    39046      +26     
   + Partials       3422     3420       -2     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/gobblin/pull/3298?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [.../org/apache/gobblin/service/ServiceConfigKeys.java](https://codecov.io/gh/apache/gobblin/pull/3298/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vc2VydmljZS9TZXJ2aWNlQ29uZmlnS2V5cy5qYXZh) | `0.00% <ø> (ø)` | |
   | [...rc/main/java/org/apache/gobblin/MysqlDagStore.java](https://codecov.io/gh/apache/gobblin/pull/3298/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1tZXRhc3RvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vTXlzcWxEYWdTdG9yZS5qYXZh) | `0.00% <0.00%> (ø)` | |
   | [...e/gobblin/metastore/MysqlDagStateStoreFactory.java](https://codecov.io/gh/apache/gobblin/pull/3298/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1tZXRhc3RvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vbWV0YXN0b3JlL015c3FsRGFnU3RhdGVTdG9yZUZhY3RvcnkuamF2YQ==) | `0.00% <0.00%> (ø)` | |
   | [...he/gobblin/metastore/MysqlJobStatusStateStore.java](https://codecov.io/gh/apache/gobblin/pull/3298/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1tZXRhc3RvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vbWV0YXN0b3JlL015c3FsSm9iU3RhdHVzU3RhdGVTdG9yZS5qYXZh) | `0.00% <0.00%> (ø)` | |
   | [.../org/apache/gobblin/metastore/MysqlStateStore.java](https://codecov.io/gh/apache/gobblin/pull/3298/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1tZXRhc3RvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vbWV0YXN0b3JlL015c3FsU3RhdGVTdG9yZS5qYXZh) | `8.16% <0.00%> (-0.04%)` | :arrow_down: |
   | [...vice/modules/orchestration/MysqlDagStateStore.java](https://codecov.io/gh/apache/gobblin/pull/3298/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9kdWxlcy9vcmNoZXN0cmF0aW9uL015c3FsRGFnU3RhdGVTdG9yZS5qYXZh) | `81.57% <75.00%> (ø)` | |
   | [.../java/org/apache/gobblin/runtime/api/FlowSpec.java](https://codecov.io/gh/apache/gobblin/pull/3298/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvYXBpL0Zsb3dTcGVjLmphdmE=) | `43.64% <100.00%> (+0.62%)` | :arrow_up: |
   | [...che/gobblin/runtime/spec\_store/MysqlSpecStore.java](https://codecov.io/gh/apache/gobblin/pull/3298/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvc3BlY19zdG9yZS9NeXNxbFNwZWNTdG9yZS5qYXZh) | `68.10% <100.00%> (+0.17%)` | :arrow_up: |
   | [...blin/service/monitoring/KafkaJobStatusMonitor.java](https://codecov.io/gh/apache/gobblin/pull/3298/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9uaXRvcmluZy9LYWZrYUpvYlN0YXR1c01vbml0b3IuamF2YQ==) | `45.45% <100.00%> (ø)` | |
   | [...he/gobblin/source/PartitionAwareFileRetriever.java](https://codecov.io/gh/apache/gobblin/pull/3298/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NvdXJjZS9QYXJ0aXRpb25Bd2FyZUZpbGVSZXRyaWV2ZXIuamF2YQ==) | `48.14% <0.00%> (-7.41%)` | :arrow_down: |
   | ... and [11 more](https://codecov.io/gh/apache/gobblin/pull/3298/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/gobblin/pull/3298?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/gobblin/pull/3298?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [75f6aff...92f183e](https://codecov.io/gh/apache/gobblin/pull/3298?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [gobblin] aplex commented on a change in pull request #3298: [GOBBLIN-1459] correct column length in MysqlSpecStore, MysqlDagStore, MysqlJobStatu…

Posted by GitBox <gi...@apache.org>.
aplex commented on a change in pull request #3298:
URL: https://github.com/apache/gobblin/pull/3298#discussion_r651945730



##########
File path: gobblin-metastore/src/main/java/org/apache/gobblin/MysqlDagStore.java
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.gobblin;
+
+import java.io.IOException;
+
+import javax.sql.DataSource;
+import lombok.extern.slf4j.Slf4j;
+
+import org.apache.gobblin.configuration.State;
+import org.apache.gobblin.metastore.MysqlStateStore;
+import org.apache.gobblin.service.ServiceConfigKeys;
+
+
+@Slf4j
+/**
+ * An implementation of {@link MysqlStateStore} backed by MySQL to store Dag.
+ *
+ * @param <T> state object type
+ **/
+public class MysqlDagStore<T extends State> extends MysqlStateStore<T> {

Review comment:
       So is this length problem only present for dag store? I would expect that if the flow name is part of the key, it will affect other state stores as well. I see that we have at least a "job_status" state store in shared Gobblin service. And if the new state store is created, it will also hit this problem.




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [gobblin] aplex commented on a change in pull request #3298: [GOBBLIN-1459] correct column length in MysqlSpecStore, MysqlDagStore, MysqlJobStatu…

Posted by GitBox <gi...@apache.org>.
aplex commented on a change in pull request #3298:
URL: https://github.com/apache/gobblin/pull/3298#discussion_r651940989



##########
File path: gobblin-metastore/src/main/java/org/apache/gobblin/MysqlDagStore.java
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.gobblin;
+
+import java.io.IOException;
+
+import javax.sql.DataSource;
+import lombok.extern.slf4j.Slf4j;
+
+import org.apache.gobblin.configuration.State;
+import org.apache.gobblin.metastore.MysqlStateStore;
+import org.apache.gobblin.service.ServiceConfigKeys;
+
+
+@Slf4j
+/**
+ * An implementation of {@link MysqlStateStore} backed by MySQL to store Dag.
+ *
+ * @param <T> state object type
+ **/
+public class MysqlDagStore<T extends State> extends MysqlStateStore<T> {
+  /**
+   * Manages the persistence and retrieval of {@link State} in a MySQL database
+   * @param dataSource the {@link DataSource} object for connecting to MySQL
+   * @param stateStoreTableName the table for storing the state in rows keyed by two levels (store_name, table_name)
+   * @param compressedValues should values be compressed for storage?
+   * @param stateClass class of the {@link State}s stored in this state store
+   * @throws IOException in case of failures
+   */
+  public MysqlDagStore(DataSource dataSource, String stateStoreTableName, boolean compressedValues,
+      Class<T> stateClass)
+      throws IOException {
+    super(dataSource, stateStoreTableName, compressedValues, stateClass);
+  }
+
+  @Override
+  protected String getCreateJobStateTableTemplate() {
+    int maxStoreName = ServiceConfigKeys.MAX_FLOW_NAME + ServiceConfigKeys.STATE_STORE_KEY_SEPARATION_CHARACTER.length()
+        + ServiceConfigKeys.MAX_FLOW_GROUP;
+    int maxTableName = 13; // length of flowExecutionId which is epoch timestamp
+
+    return "CREATE TABLE IF NOT EXISTS $TABLE$ (store_name varchar(" + maxStoreName + ") CHARACTER SET latin1 COLLATE latin1_bin not null,"

Review comment:
       ok, what would be the the actual value they need to set? Looks like its is computed




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [gobblin] codecov-commenter edited a comment on pull request #3298: [GOBBLIN-1459] correct column length in MysqlSpecStore, MysqlDagStore, MysqlJobStatu…

Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #3298:
URL: https://github.com/apache/gobblin/pull/3298#issuecomment-855118164


   # [Codecov](https://codecov.io/gh/apache/gobblin/pull/3298?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#3298](https://codecov.io/gh/apache/gobblin/pull/3298?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (92f183e) into [master](https://codecov.io/gh/apache/gobblin/commit/75f6afff6a39c01569c63ac7895ea692610fbd2c?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (75f6aff) will **decrease** coverage by `0.01%`.
   > The diff coverage is `30.30%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/gobblin/pull/3298/graphs/tree.svg?width=650&height=150&src=pr&token=4MgURJ0bGc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/gobblin/pull/3298?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #3298      +/-   ##
   ============================================
   - Coverage     46.47%   46.45%   -0.02%     
   + Complexity    10023    10021       -2     
   ============================================
     Files          2037     2039       +2     
     Lines         79294    79319      +25     
     Branches       8843     8840       -3     
   ============================================
   - Hits          36852    36848       -4     
   - Misses        39020    39055      +35     
   + Partials       3422     3416       -6     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/gobblin/pull/3298?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [.../org/apache/gobblin/service/ServiceConfigKeys.java](https://codecov.io/gh/apache/gobblin/pull/3298/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vc2VydmljZS9TZXJ2aWNlQ29uZmlnS2V5cy5qYXZh) | `0.00% <ø> (ø)` | |
   | [...rc/main/java/org/apache/gobblin/MysqlDagStore.java](https://codecov.io/gh/apache/gobblin/pull/3298/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1tZXRhc3RvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vTXlzcWxEYWdTdG9yZS5qYXZh) | `0.00% <0.00%> (ø)` | |
   | [...e/gobblin/metastore/MysqlDagStateStoreFactory.java](https://codecov.io/gh/apache/gobblin/pull/3298/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1tZXRhc3RvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vbWV0YXN0b3JlL015c3FsRGFnU3RhdGVTdG9yZUZhY3RvcnkuamF2YQ==) | `0.00% <0.00%> (ø)` | |
   | [...he/gobblin/metastore/MysqlJobStatusStateStore.java](https://codecov.io/gh/apache/gobblin/pull/3298/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1tZXRhc3RvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vbWV0YXN0b3JlL015c3FsSm9iU3RhdHVzU3RhdGVTdG9yZS5qYXZh) | `0.00% <0.00%> (ø)` | |
   | [.../org/apache/gobblin/metastore/MysqlStateStore.java](https://codecov.io/gh/apache/gobblin/pull/3298/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1tZXRhc3RvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vbWV0YXN0b3JlL015c3FsU3RhdGVTdG9yZS5qYXZh) | `8.16% <0.00%> (-0.04%)` | :arrow_down: |
   | [...vice/modules/orchestration/MysqlDagStateStore.java](https://codecov.io/gh/apache/gobblin/pull/3298/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9kdWxlcy9vcmNoZXN0cmF0aW9uL015c3FsRGFnU3RhdGVTdG9yZS5qYXZh) | `81.57% <75.00%> (ø)` | |
   | [.../java/org/apache/gobblin/runtime/api/FlowSpec.java](https://codecov.io/gh/apache/gobblin/pull/3298/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvYXBpL0Zsb3dTcGVjLmphdmE=) | `43.64% <100.00%> (+0.62%)` | :arrow_up: |
   | [...che/gobblin/runtime/spec\_store/MysqlSpecStore.java](https://codecov.io/gh/apache/gobblin/pull/3298/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvc3BlY19zdG9yZS9NeXNxbFNwZWNTdG9yZS5qYXZh) | `68.10% <100.00%> (+0.17%)` | :arrow_up: |
   | [...blin/service/monitoring/KafkaJobStatusMonitor.java](https://codecov.io/gh/apache/gobblin/pull/3298/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9uaXRvcmluZy9LYWZrYUpvYlN0YXR1c01vbml0b3IuamF2YQ==) | `45.45% <100.00%> (ø)` | |
   | [...in/java/org/apache/gobblin/cluster/HelixUtils.java](https://codecov.io/gh/apache/gobblin/pull/3298/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1jbHVzdGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NsdXN0ZXIvSGVsaXhVdGlscy5qYXZh) | `32.23% <0.00%> (-5.79%)` | :arrow_down: |
   | ... and [12 more](https://codecov.io/gh/apache/gobblin/pull/3298/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/gobblin/pull/3298?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/gobblin/pull/3298?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [75f6aff...92f183e](https://codecov.io/gh/apache/gobblin/pull/3298?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [gobblin] arjun4084346 commented on pull request #3298: [GOBBLIN-1459] correct column length in MysqlSpecStore, MysqlDagStore, MysqlJobStatu…

Posted by GitBox <gi...@apache.org>.
arjun4084346 commented on pull request #3298:
URL: https://github.com/apache/gobblin/pull/3298#issuecomment-863723356






-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [gobblin] arjun4084346 commented on a change in pull request #3298: [GOBBLIN-1459] correct column length in MysqlSpecStore, MysqlDagStore, MysqlJobStatu…

Posted by GitBox <gi...@apache.org>.
arjun4084346 commented on a change in pull request #3298:
URL: https://github.com/apache/gobblin/pull/3298#discussion_r651971999



##########
File path: gobblin-metastore/src/main/java/org/apache/gobblin/metastore/MysqlJobStatusStateStore.java
##########
@@ -70,6 +71,21 @@ public MysqlJobStatusStateStore(DataSource dataSource, String stateStoreTableNam
     return getAll(storeName, flowExecutionId + "%", true);
   }
 
+  @Override
+  protected String getCreateJobStateTableTemplate() {

Review comment:
       FYI, I added a test in other module to test long names when creating spec with invalid names.




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [gobblin] Will-Lo commented on a change in pull request #3298: [GOBBLIN-1459] correct column length in MysqlSpecStore, MysqlDagStore, MysqlJobStatu…

Posted by GitBox <gi...@apache.org>.
Will-Lo commented on a change in pull request #3298:
URL: https://github.com/apache/gobblin/pull/3298#discussion_r654042689



##########
File path: gobblin-metastore/src/main/java/org/apache/gobblin/MysqlDagStore.java
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.gobblin;
+
+import java.io.IOException;
+
+import javax.sql.DataSource;
+import lombok.extern.slf4j.Slf4j;
+
+import org.apache.gobblin.configuration.State;
+import org.apache.gobblin.metastore.MysqlStateStore;
+import org.apache.gobblin.service.ServiceConfigKeys;
+
+
+@Slf4j
+/**
+ * An implementation of {@link MysqlStateStore} backed by MySQL to store Dag.
+ *
+ * @param <T> state object type
+ **/
+public class MysqlDagStore<T extends State> extends MysqlStateStore<T> {

Review comment:
       Is there ever a scenario where we want to use the old dag state store?




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [gobblin] asfgit closed pull request #3298: [GOBBLIN-1459] correct column length in MysqlSpecStore, MysqlDagStore, MysqlJobStatu…

Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #3298:
URL: https://github.com/apache/gobblin/pull/3298


   


-- 
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: dev-unsubscribe@gobblin.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [gobblin] codecov-commenter edited a comment on pull request #3298: [GOBBLIN-1459] correct column length in MysqlSpecStore, MysqlDagStore, MysqlJobStatu…

Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #3298:
URL: https://github.com/apache/gobblin/pull/3298#issuecomment-855118164


   # [Codecov](https://codecov.io/gh/apache/gobblin/pull/3298?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#3298](https://codecov.io/gh/apache/gobblin/pull/3298?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (92f183e) into [master](https://codecov.io/gh/apache/gobblin/commit/75f6afff6a39c01569c63ac7895ea692610fbd2c?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (75f6aff) will **decrease** coverage by `37.47%`.
   > The diff coverage is `0.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/gobblin/pull/3298/graphs/tree.svg?width=650&height=150&src=pr&token=4MgURJ0bGc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/gobblin/pull/3298?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master   #3298       +/-   ##
   ============================================
   - Coverage     46.47%   8.99%   -37.48%     
   + Complexity    10023    1742     -8281     
   ============================================
     Files          2037    2039        +2     
     Lines         79294   79319       +25     
     Branches       8843    8840        -3     
   ============================================
   - Hits          36852    7135    -29717     
   - Misses        39020   71482    +32462     
   + Partials       3422     702     -2720     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/gobblin/pull/3298?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [.../org/apache/gobblin/service/ServiceConfigKeys.java](https://codecov.io/gh/apache/gobblin/pull/3298/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vc2VydmljZS9TZXJ2aWNlQ29uZmlnS2V5cy5qYXZh) | `0.00% <ø> (ø)` | |
   | [...rc/main/java/org/apache/gobblin/MysqlDagStore.java](https://codecov.io/gh/apache/gobblin/pull/3298/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1tZXRhc3RvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vTXlzcWxEYWdTdG9yZS5qYXZh) | `0.00% <0.00%> (ø)` | |
   | [...e/gobblin/metastore/MysqlDagStateStoreFactory.java](https://codecov.io/gh/apache/gobblin/pull/3298/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1tZXRhc3RvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vbWV0YXN0b3JlL015c3FsRGFnU3RhdGVTdG9yZUZhY3RvcnkuamF2YQ==) | `0.00% <0.00%> (ø)` | |
   | [...he/gobblin/metastore/MysqlJobStatusStateStore.java](https://codecov.io/gh/apache/gobblin/pull/3298/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1tZXRhc3RvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vbWV0YXN0b3JlL015c3FsSm9iU3RhdHVzU3RhdGVTdG9yZS5qYXZh) | `0.00% <0.00%> (ø)` | |
   | [.../org/apache/gobblin/metastore/MysqlStateStore.java](https://codecov.io/gh/apache/gobblin/pull/3298/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1tZXRhc3RvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vbWV0YXN0b3JlL015c3FsU3RhdGVTdG9yZS5qYXZh) | `0.00% <0.00%> (-8.20%)` | :arrow_down: |
   | [.../java/org/apache/gobblin/runtime/api/FlowSpec.java](https://codecov.io/gh/apache/gobblin/pull/3298/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvYXBpL0Zsb3dTcGVjLmphdmE=) | `3.31% <0.00%> (-39.71%)` | :arrow_down: |
   | [...che/gobblin/runtime/spec\_store/MysqlSpecStore.java](https://codecov.io/gh/apache/gobblin/pull/3298/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvc3BlY19zdG9yZS9NeXNxbFNwZWNTdG9yZS5qYXZh) | `0.00% <0.00%> (-67.94%)` | :arrow_down: |
   | [...vice/modules/orchestration/MysqlDagStateStore.java](https://codecov.io/gh/apache/gobblin/pull/3298/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9kdWxlcy9vcmNoZXN0cmF0aW9uL015c3FsRGFnU3RhdGVTdG9yZS5qYXZh) | `0.00% <0.00%> (-81.58%)` | :arrow_down: |
   | [...blin/service/monitoring/KafkaJobStatusMonitor.java](https://codecov.io/gh/apache/gobblin/pull/3298/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9uaXRvcmluZy9LYWZrYUpvYlN0YXR1c01vbml0b3IuamF2YQ==) | `0.00% <0.00%> (-45.46%)` | :arrow_down: |
   | [...c/main/java/org/apache/gobblin/util/FileUtils.java](https://codecov.io/gh/apache/gobblin/pull/3298/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi11dGlsaXR5L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3V0aWwvRmlsZVV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | ... and [1087 more](https://codecov.io/gh/apache/gobblin/pull/3298/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/gobblin/pull/3298?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/gobblin/pull/3298?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [75f6aff...92f183e](https://codecov.io/gh/apache/gobblin/pull/3298?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [gobblin] arjun4084346 commented on a change in pull request #3298: [GOBBLIN-1459] correct column length in MysqlSpecStore, MysqlDagStore, MysqlJobStatu…

Posted by GitBox <gi...@apache.org>.
arjun4084346 commented on a change in pull request #3298:
URL: https://github.com/apache/gobblin/pull/3298#discussion_r654112737



##########
File path: gobblin-metastore/src/main/java/org/apache/gobblin/MysqlDagStore.java
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.gobblin;
+
+import java.io.IOException;
+
+import javax.sql.DataSource;
+import lombok.extern.slf4j.Slf4j;
+
+import org.apache.gobblin.configuration.State;
+import org.apache.gobblin.metastore.MysqlStateStore;
+import org.apache.gobblin.service.ServiceConfigKeys;
+
+
+@Slf4j
+/**
+ * An implementation of {@link MysqlStateStore} backed by MySQL to store Dag.
+ *
+ * @param <T> state object type
+ **/
+public class MysqlDagStore<T extends State> extends MysqlStateStore<T> {

Review comment:
       Both the dag stores are actually the same. New store extends the old store overriding one method here https://github.com/apache/gobblin/pull/3298/files/d45e2c83eab8b6420fc90162ef27f1e3f8d0a9c9#diff-e44b7b6bb150dd599827d36ca60c5a08e31e406013cfb22d3b363f794899f4e8R52




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [gobblin] arjun4084346 commented on a change in pull request #3298: [GOBBLIN-1459] correct column length in MysqlSpecStore, MysqlDagStore, MysqlJobStatu…

Posted by GitBox <gi...@apache.org>.
arjun4084346 commented on a change in pull request #3298:
URL: https://github.com/apache/gobblin/pull/3298#discussion_r647812684



##########
File path: gobblin-metastore/src/main/java/org/apache/gobblin/MysqlDagStore.java
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.gobblin;
+
+import java.io.IOException;
+
+import javax.sql.DataSource;
+import lombok.extern.slf4j.Slf4j;
+
+import org.apache.gobblin.configuration.State;
+import org.apache.gobblin.metastore.MysqlStateStore;
+import org.apache.gobblin.service.ServiceConfigKeys;
+
+
+@Slf4j
+/**
+ * An implementation of {@link MysqlStateStore} backed by MySQL to store Dag.
+ *
+ * @param <T> state object type
+ **/
+public class MysqlDagStore<T extends State> extends MysqlStateStore<T> {

Review comment:
       The base class MysqlStateStore is a base class and is shared by a lot of implementations. I found it cleaner this way.




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [gobblin] arjun4084346 commented on a change in pull request #3298: [GOBBLIN-1459] correct column length in MysqlSpecStore, MysqlDagStore, MysqlJobStatu…

Posted by GitBox <gi...@apache.org>.
arjun4084346 commented on a change in pull request #3298:
URL: https://github.com/apache/gobblin/pull/3298#discussion_r647810541



##########
File path: gobblin-metastore/src/main/java/org/apache/gobblin/MysqlDagStore.java
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.gobblin;
+
+import java.io.IOException;
+
+import javax.sql.DataSource;
+import lombok.extern.slf4j.Slf4j;
+
+import org.apache.gobblin.configuration.State;
+import org.apache.gobblin.metastore.MysqlStateStore;
+import org.apache.gobblin.service.ServiceConfigKeys;
+
+
+@Slf4j
+/**
+ * An implementation of {@link MysqlStateStore} backed by MySQL to store Dag.
+ *
+ * @param <T> state object type
+ **/
+public class MysqlDagStore<T extends State> extends MysqlStateStore<T> {
+  /**
+   * Manages the persistence and retrieval of {@link State} in a MySQL database
+   * @param dataSource the {@link DataSource} object for connecting to MySQL
+   * @param stateStoreTableName the table for storing the state in rows keyed by two levels (store_name, table_name)
+   * @param compressedValues should values be compressed for storage?
+   * @param stateClass class of the {@link State}s stored in this state store
+   * @throws IOException in case of failures
+   */
+  public MysqlDagStore(DataSource dataSource, String stateStoreTableName, boolean compressedValues,
+      Class<T> stateClass)
+      throws IOException {
+    super(dataSource, stateStoreTableName, compressedValues, stateClass);
+  }
+
+  @Override
+  protected String getCreateJobStateTableTemplate() {
+    int maxStoreName = ServiceConfigKeys.MAX_FLOW_NAME + ServiceConfigKeys.STATE_STORE_KEY_SEPARATION_CHARACTER.length()
+        + ServiceConfigKeys.MAX_FLOW_GROUP;
+    int maxTableName = 13; // length of flowExecutionId which is epoch timestamp
+
+    return "CREATE TABLE IF NOT EXISTS $TABLE$ (store_name varchar(" + maxStoreName + ") CHARACTER SET latin1 COLLATE latin1_bin not null,"

Review comment:
       Let's aks SREs to increase the limit manually as it is a one time thing?

##########
File path: gobblin-metastore/src/main/java/org/apache/gobblin/MysqlDagStore.java
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.gobblin;
+
+import java.io.IOException;
+
+import javax.sql.DataSource;
+import lombok.extern.slf4j.Slf4j;
+
+import org.apache.gobblin.configuration.State;
+import org.apache.gobblin.metastore.MysqlStateStore;
+import org.apache.gobblin.service.ServiceConfigKeys;
+
+
+@Slf4j
+/**
+ * An implementation of {@link MysqlStateStore} backed by MySQL to store Dag.
+ *
+ * @param <T> state object type
+ **/
+public class MysqlDagStore<T extends State> extends MysqlStateStore<T> {
+  /**
+   * Manages the persistence and retrieval of {@link State} in a MySQL database
+   * @param dataSource the {@link DataSource} object for connecting to MySQL
+   * @param stateStoreTableName the table for storing the state in rows keyed by two levels (store_name, table_name)
+   * @param compressedValues should values be compressed for storage?
+   * @param stateClass class of the {@link State}s stored in this state store
+   * @throws IOException in case of failures
+   */
+  public MysqlDagStore(DataSource dataSource, String stateStoreTableName, boolean compressedValues,
+      Class<T> stateClass)
+      throws IOException {
+    super(dataSource, stateStoreTableName, compressedValues, stateClass);
+  }
+
+  @Override
+  protected String getCreateJobStateTableTemplate() {
+    int maxStoreName = ServiceConfigKeys.MAX_FLOW_NAME + ServiceConfigKeys.STATE_STORE_KEY_SEPARATION_CHARACTER.length()
+        + ServiceConfigKeys.MAX_FLOW_GROUP;
+    int maxTableName = 13; // length of flowExecutionId which is epoch timestamp
+
+    return "CREATE TABLE IF NOT EXISTS $TABLE$ (store_name varchar(" + maxStoreName + ") CHARACTER SET latin1 COLLATE latin1_bin not null,"

Review comment:
       Let's ask SREs to increase the limit manually as it is a one time thing?




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [gobblin] arjun4084346 commented on a change in pull request #3298: [GOBBLIN-1459] correct column length in MysqlSpecStore, MysqlDagStore, MysqlJobStatu…

Posted by GitBox <gi...@apache.org>.
arjun4084346 commented on a change in pull request #3298:
URL: https://github.com/apache/gobblin/pull/3298#discussion_r651967611



##########
File path: gobblin-metastore/src/main/java/org/apache/gobblin/metastore/MysqlJobStatusStateStore.java
##########
@@ -70,6 +71,21 @@ public MysqlJobStatusStateStore(DataSource dataSource, String stateStoreTableNam
     return getAll(storeName, flowExecutionId + "%", true);
   }
 
+  @Override
+  protected String getCreateJobStateTableTemplate() {

Review comment:
       Ok, so what should a test verify?




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [gobblin] codecov-commenter commented on pull request #3298: [GOBBLIN-1459] correct column length in MysqlSpecStore, MysqlDagStore, MysqlJobStatu…

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #3298:
URL: https://github.com/apache/gobblin/pull/3298#issuecomment-855118164


   # [Codecov](https://codecov.io/gh/apache/gobblin/pull/3298?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#3298](https://codecov.io/gh/apache/gobblin/pull/3298?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6faa154) into [master](https://codecov.io/gh/apache/gobblin/commit/75f6afff6a39c01569c63ac7895ea692610fbd2c?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (75f6aff) will **decrease** coverage by `37.47%`.
   > The diff coverage is `0.00%`.
   
   > :exclamation: Current head 6faa154 differs from pull request most recent head 92f183e. Consider uploading reports for the commit 92f183e to get more accurate results
   [![Impacted file tree graph](https://codecov.io/gh/apache/gobblin/pull/3298/graphs/tree.svg?width=650&height=150&src=pr&token=4MgURJ0bGc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/gobblin/pull/3298?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master   #3298       +/-   ##
   ============================================
   - Coverage     46.47%   8.99%   -37.48%     
   + Complexity    10023    1742     -8281     
   ============================================
     Files          2037    2039        +2     
     Lines         79294   79319       +25     
     Branches       8843    8840        -3     
   ============================================
   - Hits          36852    7135    -29717     
   - Misses        39020   71482    +32462     
   + Partials       3422     702     -2720     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/gobblin/pull/3298?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [.../org/apache/gobblin/service/ServiceConfigKeys.java](https://codecov.io/gh/apache/gobblin/pull/3298/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vc2VydmljZS9TZXJ2aWNlQ29uZmlnS2V5cy5qYXZh) | `0.00% <ø> (ø)` | |
   | [...rc/main/java/org/apache/gobblin/MysqlDagStore.java](https://codecov.io/gh/apache/gobblin/pull/3298/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1tZXRhc3RvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vTXlzcWxEYWdTdG9yZS5qYXZh) | `0.00% <0.00%> (ø)` | |
   | [...e/gobblin/metastore/MysqlDagStateStoreFactory.java](https://codecov.io/gh/apache/gobblin/pull/3298/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1tZXRhc3RvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vbWV0YXN0b3JlL015c3FsRGFnU3RhdGVTdG9yZUZhY3RvcnkuamF2YQ==) | `0.00% <0.00%> (ø)` | |
   | [...he/gobblin/metastore/MysqlJobStatusStateStore.java](https://codecov.io/gh/apache/gobblin/pull/3298/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1tZXRhc3RvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vbWV0YXN0b3JlL015c3FsSm9iU3RhdHVzU3RhdGVTdG9yZS5qYXZh) | `0.00% <0.00%> (ø)` | |
   | [.../org/apache/gobblin/metastore/MysqlStateStore.java](https://codecov.io/gh/apache/gobblin/pull/3298/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1tZXRhc3RvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vbWV0YXN0b3JlL015c3FsU3RhdGVTdG9yZS5qYXZh) | `0.00% <0.00%> (-8.20%)` | :arrow_down: |
   | [.../java/org/apache/gobblin/runtime/api/FlowSpec.java](https://codecov.io/gh/apache/gobblin/pull/3298/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvYXBpL0Zsb3dTcGVjLmphdmE=) | `3.31% <0.00%> (-39.71%)` | :arrow_down: |
   | [...che/gobblin/runtime/spec\_store/MysqlSpecStore.java](https://codecov.io/gh/apache/gobblin/pull/3298/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvc3BlY19zdG9yZS9NeXNxbFNwZWNTdG9yZS5qYXZh) | `0.00% <0.00%> (-67.94%)` | :arrow_down: |
   | [...vice/modules/orchestration/MysqlDagStateStore.java](https://codecov.io/gh/apache/gobblin/pull/3298/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9kdWxlcy9vcmNoZXN0cmF0aW9uL015c3FsRGFnU3RhdGVTdG9yZS5qYXZh) | `0.00% <0.00%> (-81.58%)` | :arrow_down: |
   | [...blin/service/monitoring/KafkaJobStatusMonitor.java](https://codecov.io/gh/apache/gobblin/pull/3298/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9uaXRvcmluZy9LYWZrYUpvYlN0YXR1c01vbml0b3IuamF2YQ==) | `0.00% <0.00%> (-45.46%)` | :arrow_down: |
   | [...c/main/java/org/apache/gobblin/util/FileUtils.java](https://codecov.io/gh/apache/gobblin/pull/3298/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi11dGlsaXR5L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3V0aWwvRmlsZVV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | ... and [1087 more](https://codecov.io/gh/apache/gobblin/pull/3298/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/gobblin/pull/3298?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/gobblin/pull/3298?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [75f6aff...92f183e](https://codecov.io/gh/apache/gobblin/pull/3298?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [gobblin] aplex commented on a change in pull request #3298: [GOBBLIN-1459] correct column length in MysqlSpecStore, MysqlDagStore, MysqlJobStatu…

Posted by GitBox <gi...@apache.org>.
aplex commented on a change in pull request #3298:
URL: https://github.com/apache/gobblin/pull/3298#discussion_r647773079



##########
File path: gobblin-runtime/src/main/java/org/apache/gobblin/runtime/api/FlowSpec.java
##########
@@ -467,5 +468,10 @@ public static FlowConfig toFlowConfig(Spec spec) {
           .setFlowName(flowProps.getProperty(ConfigurationKeys.FLOW_NAME_KEY)))
           .setProperties(flowPropsAsStringMap);
     }
+
+    public static int maxFlowSpecUri() {

Review comment:
       we can end the name with "length" or "size" to make it more clear.

##########
File path: gobblin-metastore/src/main/java/org/apache/gobblin/MysqlDagStore.java
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.gobblin;
+
+import java.io.IOException;
+
+import javax.sql.DataSource;
+import lombok.extern.slf4j.Slf4j;
+
+import org.apache.gobblin.configuration.State;
+import org.apache.gobblin.metastore.MysqlStateStore;
+import org.apache.gobblin.service.ServiceConfigKeys;
+
+
+@Slf4j
+/**
+ * An implementation of {@link MysqlStateStore} backed by MySQL to store Dag.
+ *
+ * @param <T> state object type
+ **/
+public class MysqlDagStore<T extends State> extends MysqlStateStore<T> {

Review comment:
       Do we need a separate MysqlDagStore, or we can just bump the lengths in the base class?

##########
File path: gobblin-metastore/src/main/java/org/apache/gobblin/MysqlDagStore.java
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.gobblin;
+
+import java.io.IOException;
+
+import javax.sql.DataSource;
+import lombok.extern.slf4j.Slf4j;
+
+import org.apache.gobblin.configuration.State;
+import org.apache.gobblin.metastore.MysqlStateStore;
+import org.apache.gobblin.service.ServiceConfigKeys;
+
+
+@Slf4j
+/**
+ * An implementation of {@link MysqlStateStore} backed by MySQL to store Dag.
+ *
+ * @param <T> state object type
+ **/
+public class MysqlDagStore<T extends State> extends MysqlStateStore<T> {
+  /**
+   * Manages the persistence and retrieval of {@link State} in a MySQL database
+   * @param dataSource the {@link DataSource} object for connecting to MySQL
+   * @param stateStoreTableName the table for storing the state in rows keyed by two levels (store_name, table_name)
+   * @param compressedValues should values be compressed for storage?
+   * @param stateClass class of the {@link State}s stored in this state store
+   * @throws IOException in case of failures
+   */
+  public MysqlDagStore(DataSource dataSource, String stateStoreTableName, boolean compressedValues,
+      Class<T> stateClass)
+      throws IOException {
+    super(dataSource, stateStoreTableName, compressedValues, stateClass);
+  }
+
+  @Override
+  protected String getCreateJobStateTableTemplate() {
+    int maxStoreName = ServiceConfigKeys.MAX_FLOW_NAME + ServiceConfigKeys.STATE_STORE_KEY_SEPARATION_CHARACTER.length()
+        + ServiceConfigKeys.MAX_FLOW_GROUP;
+    int maxTableName = 13; // length of flowExecutionId which is epoch timestamp
+
+    return "CREATE TABLE IF NOT EXISTS $TABLE$ (store_name varchar(" + maxStoreName + ") CHARACTER SET latin1 COLLATE latin1_bin not null,"

Review comment:
       Looks like we'll use longer column names when new state stores are created, but what is our story for the existing DBs? Are we going to ask SREs to increase the limit manually, or we'll include some "alter table" in the codebase?

##########
File path: gobblin-api/src/main/java/org/apache/gobblin/service/ServiceConfigKeys.java
##########
@@ -132,4 +132,11 @@
   // Group Membership authentication service
   public static final String GROUP_OWNERSHIP_SERVICE_CLASS = GOBBLIN_SERVICE_PREFIX + "groupOwnershipService.class";
   public static final String DEFAULT_GROUP_OWNERSHIP_SERVICE = "org.apache.gobblin.service.NoopGroupOwnershipService";
+
+  public static final int MAX_FLOW_NAME = 128; // defined in FlowId.pdl

Review comment:
       maybe MAX_FLOW_NAME_LENGTH? and same for the next config.

##########
File path: gobblin-metastore/src/main/java/org/apache/gobblin/metastore/MysqlJobStatusStateStore.java
##########
@@ -70,6 +71,21 @@ public MysqlJobStatusStateStore(DataSource dataSource, String stateStoreTableNam
     return getAll(storeName, flowExecutionId + "%", true);
   }
 
+  @Override
+  protected String getCreateJobStateTableTemplate() {

Review comment:
       Would be good to add a couple of tests that will verify the state store behavior with long names. Older version of MySql with a certain db layout types would limit the total length of the primary key to about 700-800 bytes. At the same time, it was possible to define any limits in table schema, even more than 800 total bytes. But, when you try to insert a long key, a runtime exception will occur.




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org