Added Table Filtering #19

abhimanyugupta07 · 2018-12-07T07:53:36Z

refer #6

...r/src/main/java/com/hotels/shunting/yard/replicator/exec/messaging/MessageReaderAdapter.java

patduin · 2018-12-07T09:43:26Z

...d-replicator/src/main/java/com/hotels/shunting/yard/replicator/exec/context/CommonBeans.java

    MessageReaderFactory messaReaderFactory = MessageReaderFactory
        .newInstance(messageReaderConfig.getMessageReaderFactoryClass());
    MessageReader messageReader = messaReaderFactory.newInstance(replicaHiveConf, sqsMessageSerDe);
-    return new MessageReaderAdapter(messageReader, sourceCatalog.getHiveMetastoreUris());
+    return new MessageReaderAdapter(messageReader, sourceCatalog.getHiveMetastoreUris(), tableSelector);


I think if possible the Adapter should not know about the filtering of tables it should get ListenerEvents that are valid for processing. Perhaps we need to look at a FilteringMessageReader that filters out the stream of event. Can be a decarator on whaterever MessageReader implementation we use currently?

I like the idea of using a decorator to filter out the tables we're not interested in.

massdosage · 2018-12-07T16:09:17Z

shunting-yard-binary/src/main/scripts/replicator.sh

@@ -33,8 +33,8 @@ fi
 LIBFB303_JAR=`ls $HIVE_LIB/libfb303-*.jar | tr '\n' ':'`

 SHUNTING_YARD_LIBS=$SHUNTING_YARD_HOME/lib/*\
-:$HIVE_LIB/hive-exec.jar\
-:$HIVE_LIB/hive-metastore.jar\
+:$HIVE_LIB/hive-exec*.jar\


Why do we need this? I'm guessing we have version numbers in some situations? (just curious what those situations are).

Discussed, this is due to difference between EMR and Docker, possibly worth looking at the different jars and seeing if there is some way to craft this so it works in both setups.

massdosage · 2018-12-07T16:15:59Z

...eplicator/src/main/java/com/hotels/shunting/yard/replicator/exec/conf/TargetReplication.java

+import org.springframework.context.annotation.Configuration;
+
+@Configuration
+@ConfigurationProperties(prefix = "target-replication")


This matches the layout in the YAML right? Where it makes sense I think we should re-use the same names as we use in Circus Train, so I was thinking more like something like this:

table-replications: - source-table: database-name: foo table-name: bar

i.e. in the YAML file you can provide a list of tables to replicate. We could also allow regex to be used for both the db and table name and then compile this into a list of potential matches.

massdosage · 2018-12-07T16:18:02Z

...d-replicator/src/main/java/com/hotels/shunting/yard/replicator/exec/context/CommonBeans.java

    MessageReaderFactory messaReaderFactory = MessageReaderFactory
        .newInstance(messageReaderConfig.getMessageReaderFactoryClass());
    MessageReader messageReader = messaReaderFactory.newInstance(replicaHiveConf, sqsMessageSerDe);
-    return new MessageReaderAdapter(messageReader, sourceCatalog.getHiveMetastoreUris());
+    return new MessageReaderAdapter(messageReader, sourceCatalog.getHiveMetastoreUris(), tableSelector);


I like the idea of using a decorator to filter out the tables we're not interested in.

massdosage · 2018-12-07T16:20:19Z

...eplicator/src/main/java/com/hotels/shunting/yard/replicator/exec/receiver/TableSelector.java

+  }
+
+  public boolean canProcess(ListenerEvent listenerEvent) {
+    List<String> tableNames = targetReplication.getTableNames();


You can do this in the constructor and keep a reference to the tables to match rather than the targetReplication itself (or perhaps even better, the constructor should just take a list and not be coupled to the targetReplication at all).

coveralls · 2018-12-08T06:29:38Z

Pull Request Test Coverage Report for Build 185

50 of 67 (74.63%) changed or added relevant lines in 9 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.6%) to 72.075%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
shunting-yard-replicator/src/main/java/com/hotels/shunting/yard/replicator/exec/messaging/AggregatingMetaStoreEventReader.java	10	11	90.91%
shunting-yard-replicator/src/main/java/com/hotels/shunting/yard/replicator/exec/messaging/MessageReaderAdapter.java	3	4	75.0%
shunting-yard-replicator/src/main/java/com/hotels/shunting/yard/replicator/exec/messaging/FilteringMessageReader.java	8	10	80.0%
shunting-yard-replicator/src/main/java/com/hotels/shunting/yard/replicator/exec/context/CommonBeans.java	0	3	0.0%
shunting-yard-replicator/src/main/java/com/hotels/shunting/yard/replicator/exec/MetaStoreEventReplication.java	0	10	0.0%

Totals
Change from base Build 148:	0.6%
Covered Lines:	733
Relevant Lines:	1017

💛 - Coveralls

patduin · 2019-01-03T08:45:42Z

...test/java/com/hotels/shunting/yard/replicator/exec/messaging/FilteringMessageReaderTest.java

+  private @Mock ListenerEvent listenerEvent1;
+  private @Mock ListenerEvent listenerEvent2;
+  private @Mock ListenerEvent listenerEvent3;
+  private @Mock SqsMessageReader delegate;


SqsMessageReader -> MessageReader No need to mock the concrete class

patduin · 2019-01-03T08:51:26Z

...test/java/com/hotels/shunting/yard/replicator/exec/messaging/FilteringMessageReaderTest.java

+    when(listenerEvent3.getDbName()).thenReturn(DB_NAME);
+    when(listenerEvent3.getTableName()).thenReturn(TABLE_NAME3);
+
+    when(delegate.hasNext()).thenReturn(true);


should probably mock this in a way that it respects the MessageReader interface. So hasNext should return true if there is a next otherwise false

patduin · 2019-01-03T08:53:29Z

...test/java/com/hotels/shunting/yard/replicator/exec/messaging/FilteringMessageReaderTest.java

+    filteringMessageReader = new FilteringMessageReader(delegate, tableSelector);
+    ListenerEvent event = null;
+
+    for (int i = 0; i < 2; i++) {


this is a bit weird perhaps

assertThat(filteringMessageReader.hasNext(), is(true)); ListenerEvent event = filteringMessageReader.next(); assertThat(event.getDbName()).isEqualTo(DB_NAME); assertThat(event.getTableName()).isEqualTo(TABLE_NAME1); assertThat(filteringMessageReader.hasNext(), is(true)); event = filteringMessageReader.next(); assertThat(event.getDbName()).isEqualTo(DB_NAME); assertThat(event.getTableName()).isEqualTo(TABLE_NAME2); assertThat((filteringMessageReader.hasNext(), is(false));

patduin · 2019-01-03T12:53:05Z

...src/main/java/com/hotels/shunting/yard/replicator/exec/messaging/FilteringMessageReader.java

+        return true;
+      }
+    }
+    return false;


Probably best to set current to null before returning

patduin · 2019-01-04T08:37:13Z

...test/java/com/hotels/shunting/yard/replicator/exec/messaging/FilteringMessageReaderTest.java

+    when(delegate.hasNext()).thenReturn(false);
+    filteringMessageReader = new FilteringMessageReader(delegate, tableSelector);
+
+    assertThat(filteringMessageReader.hasNext(), is(false));


assert that the next() == null?

patduin · 2019-01-04T08:49:58Z

Add a line to the changelog please and once we settle on how the config should look we maybe need to add a short section in the README on how to use it the feature.

…in FilteringMessageReader

patduin · 2019-01-04T14:22:08Z

...src/main/java/com/hotels/shunting/yard/replicator/exec/messaging/FilteringMessageReader.java

@@ -32,6 +33,12 @@ public FilteringMessageReader(MessageReader delegate, TableSelector tableSelecto
    this.tableSelector = tableSelector;
  }

+  /**


remove this, this is not true. This class has nothing to do with the SQSMessageReader. This filtering will work for any implementation.
The MessageReader extends Iterator so hasNext will not be removed in the future. We just follow the iterator contract. See https://docs.oracle.com/javase/7/docs/api/java/util/Iterator.html.

Concerning the SQSMessageReader I understand return true is confusing but it does not break the contract. It doesn't say you cannot wait indefinitely for next to return a new value it just says: "return true if the iterator has more elements"

patduin · 2019-01-04T14:24:42Z

...src/main/java/com/hotels/shunting/yard/replicator/exec/messaging/FilteringMessageReader.java

+
+  @Override
+  public ListenerEvent next() {
+    return current;


Looking at the Iterator contract in more depth we should do:

if (current == null) { throw new NoSuchElementException(); } return current;

- use Optional and remove Iterator - update license headers - added legal section - added log4j config for tests

Adding table Selector

1a1356b

patduin reviewed Dec 7, 2018

View reviewed changes

...r/src/main/java/com/hotels/shunting/yard/replicator/exec/messaging/MessageReaderAdapter.java Outdated Show resolved Hide resolved

patduin reviewed Dec 7, 2018

View reviewed changes

massdosage suggested changes Dec 7, 2018

View reviewed changes

Updating Table Filter implementation

1af391b

patduin reviewed Jan 3, 2019

View reviewed changes

Adding one more test and fixing CR suggestions

b6ee7e8

patduin reviewed Jan 3, 2019

View reviewed changes

abhimanyugupta07 added 2 commits January 3, 2019 15:36

Changing YAML tag name

769e6e8

removing a System out

0c780da

patduin reviewed Jan 4, 2019

View reviewed changes

abhimanyugupta07 added 2 commits January 4, 2019 13:39

CR comments incorporated along with a comment for hasNext() function …

37ef483

…in FilteringMessageReader

updating Changelog for issue-6

15ac7a7

patduin reviewed Jan 4, 2019

View reviewed changes

massdosage and others added 3 commits January 8, 2019 13:11

Table filter with better behaving "hasNext()" (#20)

41c60e3

- use Optional and remove Iterator - update license headers - added legal section - added log4j config for tests

reduce scope of synchronized block

d20a43a

Removing catch all character from replicator.sh

81b697a

massdosage approved these changes Jan 8, 2019

View reviewed changes

Update README and template files

c3693e6

patduin approved these changes Jan 8, 2019

View reviewed changes

abhimanyugupta07 merged commit 7f6988c into master Jan 8, 2019

abhimanyugupta07 deleted the table-filter branch January 8, 2019 15:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Table Filtering #19

Added Table Filtering #19

abhimanyugupta07 commented Dec 7, 2018

patduin Dec 7, 2018

massdosage Dec 7, 2018

massdosage Dec 7, 2018

massdosage Jan 4, 2019

massdosage Dec 7, 2018

massdosage Dec 7, 2018

massdosage Dec 7, 2018

coveralls commented Dec 8, 2018 •

edited

Loading

patduin Jan 3, 2019

patduin Jan 3, 2019

patduin Jan 3, 2019 •

edited

Loading

patduin Jan 3, 2019

patduin Jan 4, 2019

patduin commented Jan 4, 2019

patduin Jan 4, 2019 •

edited

Loading

patduin Jan 4, 2019

Added Table Filtering #19

Added Table Filtering #19

Conversation

abhimanyugupta07 commented Dec 7, 2018

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

coveralls commented Dec 8, 2018 • edited Loading

Pull Request Test Coverage Report for Build 185

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patduin Jan 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patduin commented Jan 4, 2019

patduin Jan 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 8, 2018 •

edited

Loading

patduin Jan 3, 2019 •

edited

Loading

patduin Jan 4, 2019 •

edited

Loading