Issue 173 struct evolution #174

max-jacobs · 2020-03-10T16:35:10Z

Addresses #173

...in-core/src/main/java/com/hotels/bdp/circustrain/core/replica/hive/RenameTableOperation.java

abhimanyugupta07 · 2020-03-11T10:55:06Z

...src/test/java/com/hotels/bdp/circustrain/integration/CircusTrainHdfsHdfsIntegrationTest.java

+        "struct<name:string, city:string>",
+        structData,
+        1);
+    LOG.info(">>>> Table {} ", replicaCatalog.client().getTable(DATABASE, PARTITIONED_TABLE));


This was for debugging ?

Hmm.. I see it in all the other tests too. So, may be it has some purpose. @patduin ?

Yeah its in all the tests, think its just for logging purposes. It's kind of handy, especially when checking a build

I think it's just for logging, you can leave it if you find it handy, I'm ok either way

abhimanyugupta07 · 2020-03-11T10:55:18Z

...src/test/java/com/hotels/bdp/circustrain/integration/CircusTrainHdfsHdfsIntegrationTest.java

+        "struct<name:string, city:string, dob:string>",
+        structData,
+        2);
+    LOG.info(">>>> Table {} ", sourceCatalog.client().getTable(DATABASE, PARTITIONED_TABLE));


this one too.

patduin · 2020-03-11T11:57:25Z

...train-core/src/main/java/com/hotels/bdp/circustrain/core/replica/hive/AlterTableService.java

+      LOG
+          .info("Found columns that have changed type, attempting to recreate target table with the new columns."
+              + "Old columns: {}, new columns: {}", oldColumns, newColumns);
+      Table tempTable = new Table(newTable);


Does this copy all the table properties?

It does indeed. We can either (1) scrub all the table params here, or (2) I've created a DropTableService which removes custom table params before calling the drop table command. If thats overly complicated I can remove the new service and just do (1) instead.

OK so if we removed the params here then we don't have to worry about the potential of Beekeeper picking up the drop of the temp table etc.? I think I prefer that approach unless some of the params are useful to have on the temp table? I don't think any of the intermediate operations use them in any way so probably not? And to be clear, the original table params will be kept on the final end result table?

Okay cool, I'm happy to do this, I'll remove the DropTableService, and just remove the params as part of this service.

The intermediate ops dont use the params, but i think beekeeper may pick up on the alter partition events (though will ignore them as the location doesnt change). And yes, the original params will be on the final table - i'll add some tests for this.

Tried this out, and actually it doesn't seem possible. We can't do an alter table on the table as it still has the old schema, so it fails with the same error. So the DropTableService looks better as we do the alter/drop once everything has been updated.

OK, let's go with that for now and keep an eye on it when this is in use.

patduin · 2020-03-11T12:03:40Z

...in-core/src/main/java/com/hotels/bdp/circustrain/core/replica/hive/RenameTableOperation.java

+      client.alter_table(to.getDbName(), toTableName, to);
+      client.alter_table(from.getDbName(), fromTableName, from);
+    } finally {
+      client.dropTable(to.getDbName(), toTableNameTemp, false, true);


We need to double check beekeeper props and we don't accidentally delete folders when we drop this table.

…dropping

...train-core/src/main/java/com/hotels/bdp/circustrain/core/replica/hive/AlterTableService.java

massdosage · 2020-03-11T16:25:55Z

circus-train-core/src/main/java/com/hotels/bdp/circustrain/core/replica/ReplicaFactory.java

@@ -49,19 +55,24 @@ public ReplicaFactory(
      Supplier<CloseableMetaStoreClient> replicaMetaStoreClientSupplier,
      HousekeepingListener housekeepingListener,
      ReplicaCatalogListener replicaCatalogListener,
-      ReplicaTableFactoryProvider replicaTableFactoryPicker) {
+      ReplicaTableFactoryProvider replicaTableFactoryPicker,


Suggested change

ReplicaTableFactoryProvider replicaTableFactoryPicker,

ReplicaTableFactoryProvider replicaTableFactoryProvider,

max-jacobs · 2020-03-11T16:47:04Z

...in-core/src/main/java/com/hotels/bdp/circustrain/core/replica/hive/RenameTableOperation.java

+    String toTableName = toTable.getTableName();
+    try {
+      fromTable.setTableName(toTableName);
+      toTable.setTableName(toTableName + TEMP_SUFFIX);


to be deleted?

massdosage · 2020-03-12T17:08:31Z

...n-core/src/test/java/com/hotels/bdp/circustrain/core/replica/hive/AlterTableServiceTest.java

+    }
+  }
+
+  private StorageDescriptor getSd(List<FieldSchema> columns) {


Suggested change

private StorageDescriptor getSd(List<FieldSchema> columns) {

private StorageDescriptor createStorageDescriptor(List<FieldSchema> columns) {

massdosage · 2020-03-12T17:09:04Z

.../src/test/java/com/hotels/bdp/circustrain/core/replica/hive/CopyPartitionsOperationTest.java

+  }
+
+  private List<Partition> createPartitions(int count) {
+    ArrayList<Partition> partitions = new ArrayList<>();


Suggested change

ArrayList<Partition> partitions = new ArrayList<>();

List<Partition> partitions = new ArrayList<>();

massdosage · 2020-03-12T17:09:35Z

...in-core/src/test/java/com/hotels/bdp/circustrain/core/replica/hive/DropTableServiceTest.java

+
+  @Test
+  public void removeParamsAndDrop() throws TException {
+    HashMap<String, String> params = new HashMap<>();


Suggested change

HashMap<String, String> params = new HashMap<>();

Map<String, String> params = new HashMap<>();

massdosage · 2020-03-12T17:09:44Z

...in-core/src/test/java/com/hotels/bdp/circustrain/core/replica/hive/DropTableServiceTest.java

+
+  @Test
+  public void removeParamsAndDropCaseInsensitiveExternalTable() throws TException {
+    HashMap<String, String> params = new HashMap<>();


Suggested change

HashMap<String, String> params = new HashMap<>();

Map<String, String> params = new HashMap<>();

massdosage · 2020-03-12T17:11:23Z

circus-train-integration-tests/pom.xml

+      <artifactId>avro</artifactId>
+      <version>1.9.0</version>
+    </dependency>
+    <dependency>


Why did you move this down here? Ideally the dependencies should be alpha sorted by groupId and then artifactId in two sections - one for normal scope and another for test scope.

Think this was out of desperation initially to get the parquet files writing successfully. Will revert.

massdosage · 2020-03-12T17:12:06Z

circus-train-integration-tests/pom.xml

@@ -172,6 +165,28 @@
      <artifactId>protobuf-java</artifactId>
      <scope>test</scope>
    </dependency>
+    <dependency>


I'd be wary of setting these to version numbers other than what the Hive/Hadoop dependencies are using. I guess this works but I just know how fragile things can get as the newer versions aren't backwards compatible and the only reason we're not seeing an issue now is we haven't hit certain code paths that would trigger problems.

massdosage · 2020-03-12T17:12:32Z

...in/integration/CircusTrainHdfsHdfsIntegrationTest/partitioned-single-table-one-partition.yml

+  preserve-raw-xattrs: false
+metrics-reporter:
+  period: 1
+  time-unit: SECONDS


massdosage · 2020-03-12T17:12:49Z

...src/test/java/com/hotels/bdp/circustrain/integration/CircusTrainHdfsHdfsIntegrationTest.java

+        .noDefault()
+        .endRecord();
+
+    HashMap<String, String> structData = new HashMap<>();


Suggested change

HashMap<String, String> structData = new HashMap<>();

Map<String, String> structData = new HashMap<>();

massdosage · 2020-03-12T17:13:38Z

...ration-tests/src/test/java/com/hotels/bdp/circustrain/integration/IntegrationTestHelper.java

+    try {
+      writer.write(record);
+    } catch (IOException e) {
+      e.printStackTrace();


Don't we want to throw this and fail the test if this happens?

massdosage · 2020-03-13T12:17:30Z

...train-core/src/main/java/com/hotels/bdp/circustrain/core/replica/hive/AlterTableService.java

+      LOG
+          .info("Found columns that have changed type, attempting to recreate target table with the new columns."
+              + "Old columns: {}, new columns: {}", oldColumns, newColumns);
+      Table tempTable = new Table(newTable);


OK, let's go with that for now and keep an eye on it when this is in use.

massdosage · 2020-03-13T12:23:50Z

...ore/src/test/java/com/hotels/bdp/circustrain/core/replica/hive/RenameTableOperationTest.java

+  }
+
+  @Test
+  public void renameTableExceptionThrown1() throws Exception {


NABD but would be nice to replace "1" and "2" in this and the next method with something more descriptive as to what the difference is. From what I can see it's about where the exception is thrown, when altering the to or the from table? So maybe something like renameToTableException and renameFromTableException?

Max Jacobs added 10 commits March 5, 2020 16:41

Initial commit with flakey test

25560f7

Updates to test

0000ac5

Updates to test

42dc731

Adding column in between two columns

2d0d100

Some changes to use Parquet

56ab02c

Test now failing successfully

85b74ba

Creating AlterTableService and operations

a548c95

Fixing integration test

0f6ff36

Fixing integration test

c304abc

Fixing unit tests and copyright

8638af1

max-jacobs requested a review from a team March 10, 2020 16:35

patduin reviewed Mar 10, 2020

View reviewed changes

...in-core/src/main/java/com/hotels/bdp/circustrain/core/replica/hive/RenameTableOperation.java Outdated Show resolved Hide resolved

Some fixes

8d5eded

abhimanyugupta07 reviewed Mar 11, 2020

View reviewed changes

patduin reviewed Mar 11, 2020

View reviewed changes

Max Jacobs added 3 commits March 11, 2020 14:40

Adding DropTableService which removes custom table parameters before …

12aa815

…dropping

Adding changelog and other bits

e1b2eac

Added catch for non existent table

4ce2b90

massdosage suggested changes Mar 11, 2020

View reviewed changes

max-jacobs commented Mar 11, 2020

View reviewed changes

Max Jacobs added 2 commits March 12, 2020 11:54

Fixing some bits, renaming, adding external key to table when dropping

7ee9d23

Undoing unnecessary changes

94ea2ee

massdosage suggested changes Mar 12, 2020

View reviewed changes

Max Jacobs added 2 commits March 13, 2020 11:12

Fixes

890ea8d

Fixing pom

2b93368

massdosage approved these changes Mar 13, 2020

View reviewed changes

Fixing pom and test names

2b2664f

patduin approved these changes Mar 13, 2020

View reviewed changes

Minor change

e8cd3a8

Max Jacobs added 2 commits March 16, 2020 18:20

Merge branch 'master' into issue-173-struct-evolution

4e40d29

Fixing tests

971e669

max-jacobs merged commit f6f5389 into master Mar 16, 2020

max-jacobs deleted the issue-173-struct-evolution branch March 16, 2020 19:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 173 struct evolution #174

Issue 173 struct evolution #174

max-jacobs commented Mar 10, 2020

abhimanyugupta07 Mar 11, 2020

abhimanyugupta07 Mar 11, 2020

max-jacobs Mar 11, 2020

patduin Mar 11, 2020

abhimanyugupta07 Mar 11, 2020

patduin Mar 11, 2020

max-jacobs Mar 11, 2020

massdosage Mar 12, 2020

max-jacobs Mar 13, 2020

max-jacobs Mar 13, 2020

massdosage Mar 13, 2020

patduin Mar 11, 2020

massdosage Mar 11, 2020

max-jacobs Mar 12, 2020

max-jacobs Mar 11, 2020

massdosage Mar 12, 2020

massdosage Mar 12, 2020

massdosage Mar 12, 2020

massdosage Mar 12, 2020

massdosage Mar 12, 2020

max-jacobs Mar 13, 2020

massdosage Mar 12, 2020

massdosage Mar 12, 2020

massdosage Mar 12, 2020

massdosage Mar 12, 2020

massdosage Mar 13, 2020

massdosage Mar 13, 2020

	ReplicaTableFactoryProvider replicaTableFactoryPicker,
	ReplicaTableFactoryProvider replicaTableFactoryProvider,

	private StorageDescriptor getSd(List<FieldSchema> columns) {
	private StorageDescriptor createStorageDescriptor(List<FieldSchema> columns) {

	ArrayList<Partition> partitions = new ArrayList<>();
	List<Partition> partitions = new ArrayList<>();

	HashMap<String, String> params = new HashMap<>();
	Map<String, String> params = new HashMap<>();

	HashMap<String, String> structData = new HashMap<>();
	Map<String, String> structData = new HashMap<>();

Issue 173 struct evolution #174

Issue 173 struct evolution #174

Conversation

max-jacobs commented Mar 10, 2020

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment