New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

HIVE-25900: Materialized view registry does not clean non existing views at refresh #2984

Closed

kasakrisz wants to merge 15 commits into apache:master from kasakrisz:HIVE-25900-master-mv-reg-refresh

Contributor

kasakrisz commented Jan 27, 2022 •

edited

What changes were proposed in this pull request?

Split implementation of HiveMaterializedViewsRegistry to a dummy DummyMaterializedViewsRegistry and the default InMemoryMaterializedViewsRegistry
Add functionality to remove dropped MVs from cache at refresh
Add unit tests for Refresh algorithm

Why are the changes needed?

Dropped MVs stayed forver in the cache if the Drop operation was done by a different HS2 instance.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

mvn test -Dtest.output.overwrite -DskipSparkTests -Dtest=TestMiniLlapLocalCliDriver -Dqfile=materialized_view_create_rewrite_rebuild_dummy.q,materialized_view_create_rewrite_dummy.q -pl itests/qtest -Pitests
mvn test -Dtest=TestHiveMaterializedViewsRegistryLoader -pl ql

kasakrisz requested a review from kgyrtkirk

January 27, 2022 16:59

kasakrisz self-assigned this

kgyrtkirk added the tests pending label

kasakrisz requested a review from zabetak

January 27, 2022 16:59

kgyrtkirk added tests unstable tests pending tests failed and removed tests pending tests unstable labels

kasakrisz force-pushed the HIVE-25900-master-mv-reg-refresh branch from e4c58a2 to 4c5ebe7 Compare

January 28, 2022 08:46

kgyrtkirk added tests pending and removed tests failed labels

zabetak reviewed

View reviewed changes

Contributor

zabetak left a comment

Thanks for working on this @kasakrisz . I went over the PR and left some comments.

While reviewing this part of code I get the impression that the registry/cache is not thread safe. It also seems that multiple threads do perform operations on this registry/cache concurrently which makes me think that we should invest in making it thread safe.

The changes in this PR stem from the fact that we may end up with an OOM cause we do not invalidate the entries in the cache. Cache invalidation though could come almost for free if we used some existing cache implementation (such as those provided by Guava). The additional benefit is that most existing cache implementations are thread-safe by design.

ql/src/java/org/apache/hadoop/hive/ql/metadata/HiveMaterializedViewsRegistry.java Outdated

Comment on lines 101 to 106

+                /**
+                 * Registry for materialized views. The goal of this cache is to avoid parsing and creating
+                 * logical plans for the materialized views at query runtime. When a query arrives, we will
+                 * just need to consult this cache and extract the logical plans for the views (which had
+                 * already been parsed) from it. This cache lives in HS2.
+                 */

Contributor

zabetak Jan 28, 2022

It is a bit a confusing that we have the same javadoc twice (here and in the top level class).

Contributor Author

kasakrisz Jan 31, 2022

removed one of them

ql/src/java/org/apache/hadoop/hive/ql/metadata/HiveMaterializedViewsRegistry.java Outdated

+                 * just need to consult this cache and extract the logical plans for the views (which had
+                 * already been parsed) from it. This cache lives in HS2.
+                 */
+                public interface MaterializedViewsRegistry {

Contributor

zabetak Jan 28, 2022

Why do we need to have both a class (HiveMaterializedViewsRegistry) and an inner interface (MaterializedViewsRegistry)? This pattern is not very easy to follow and it is not clear what are the responsibilities of each.

Contributor Author

kasakrisz Jan 31, 2022

The intention was to extract the Dummy and Default implementations to separate classes. The MaterializedViewsRegistry interface is the common interface of the two implementations. Moved it to a separate file.

ql/src/java/org/apache/hadoop/hive/ql/metadata/HiveMaterializedViewsRegistry.java Outdated

-                private final MaterializedViewsCache materializedViewsCache = new MaterializedViewsCache();
+                  /**
+                   * Adds a newly created materialized view to the cache.

Contributor

zabetak Jan 28, 2022

NIT: Use "cache" or "registry" but not both. Probably we should follow the name of the class/interface.

It seems that some methods mention registry while others mention cache.

Contributor Author

kasakrisz Jan 31, 2022

Changed cache to registry, since this is more like a registry.

ql/src/java/org/apache/hadoop/hive/ql/metadata/HiveMaterializedViewsRegistry.java Outdated

Comment on lines 145 to 149

+                  /**
+                   * Returns the materialized views in the cache for the given database.
+                   *
+                   * @return the collection of materialized views, or the empty collection if none
+                   */

Contributor

zabetak Jan 28, 2022

The javadoc does not correspond to what this method does.

Contributor Author

kasakrisz Jan 31, 2022

Fixed

ql/src/java/org/apache/hadoop/hive/ql/metadata/HiveMaterializedViewsRegistry.java Outdated

Comment on lines 155 to 161

+                  default List<HiveRelOptMaterialization> getRewritingMaterializedViews(String querySql) {
+                    return Collections.emptyList();
+                  }
+                  default boolean isEmpty() {
+                    return true;
+                  }

Contributor

zabetak Jan 28, 2022

Missing javadoc

Contributor Author

kasakrisz Jan 31, 2022

Added.

ql/src/java/org/apache/hadoop/hive/ql/metadata/HiveMaterializedViewsRegistry.java Outdated

Comment on lines 229 to 163

+                public interface MaterializedViewObjects {
+                  List<Table> getAllMaterializedViewObjectsForRewriting() throws HiveException;
+                }

Contributor

zabetak Jan 28, 2022

The new interface gives the impression that we are adding complexity instead of simplifying things. Keeping a List of tables looks simpler.

Contributor Author

kasakrisz Jan 31, 2022

The intent was to make the Loader testable. The MaterializedViewObjects represents the HMS client. The interface enables us to mock it in tests. Please see TestHiveMaterializedViewsRegistryLoader.

By default the getAllMaterializedViewObjectsForRewriting method is called periodically by the registry.

Could you please show an example with List of tables?

ql/src/java/org/apache/hadoop/hive/ql/metadata/HiveMaterializedViewsRegistry.java Outdated

+                  protected final HiveConf hiveConf;
+                  protected final MaterializedViewsRegistry materializedViewsRegistry;
+                  protected final MaterializedViewObjects materializedViewObjects;
+                  /* Whether the cache has been initialized or not. */

Contributor

zabetak Jan 28, 2022

Bogus comment?

Suggested change

      
                /* Whether the cache has been initialized or not. */
          
                /* Whether the cache has been initialized or not. */

Contributor Author

kasakrisz Jan 31, 2022

removed

ql/src/java/org/apache/hadoop/hive/ql/metadata/HiveMaterializedViewsRegistry.java Outdated

-                  private Loader(Hive db) {
-                    this.db = db;
+                public static class Loader implements Runnable {

Contributor

zabetak Jan 28, 2022

Does the class need to be public?

Contributor Author

kasakrisz Jan 31, 2022

Changed to package private.

ql/src/java/org/apache/hadoop/hive/ql/metadata/HiveMaterializedViewsRegistry.java

+                      for (HiveRelOptMaterialization materialization : materializedViewsRegistry.getRewritingMaterializedViews()) {
+                        Table mvTableInCache = HiveMaterializedViewUtils.extractTable(materialization);
+                        Table mvTableInHMS = materializedViewObjects.stream()

Contributor

zabetak Jan 28, 2022

How do we keep materializedViewObjects up-to-date with the HMS? It seems that we obtain the views once when the Loader is created but then I don't see it changed.

Contributor Author

kasakrisz Jan 31, 2022

Update is scheduled at a fixed rate:

pool.scheduleAtFixedRate(new Loader(db.getConf(), SINGLETON, objects), 0, period, TimeUnit.SECONDS);

ql/src/java/org/apache/hadoop/hive/ql/metadata/HiveMaterializedViewsRegistry.java

Comment on lines +294 to +230

+                  private PerfLogger getPerfLogger() {
+                    SessionState ss = new SessionState(hiveConf);
+                    ss.setIsHiveServerQuery(true); // All is served from HS2, we do not need e.g. Tez sessions
+                    SessionState.start(ss);
+                    return SessionState.getPerfLogger();
+                  }

Contributor

zabetak Jan 28, 2022

Do we really need the perf logger? Using a session just to obtain a logger seems a bit of an overkill.

Contributor Author

kasakrisz Jan 31, 2022

This was added by https://issues.apache.org/jira/browse/HIVE-21344 and I think the intention was to monitor the refresh rate.

kgyrtkirk added tests unstable and removed tests pending labels

kasakrisz force-pushed the HIVE-25900-master-mv-reg-refresh branch from 4c5ebe7 to bc1be69 Compare

January 31, 2022 06:30

kgyrtkirk added tests pending tests failed tests unstable and removed tests unstable tests pending tests failed labels

kasakrisz force-pushed the HIVE-25900-master-mv-reg-refresh branch from f1a0a5e to 733f011 Compare

February 1, 2022 09:13

kgyrtkirk added tests pending tests failed and removed tests unstable tests pending labels

kasakrisz added 15 commits

February 2, 2022 07:50


          HIVE-25900: Materialized view registry does not clean non existing vi…

53ca6f8

…ews at refresh


          switch off automatic cache refresh

5114bf6


          add UT for Loader

3ba44fe


          remove DummyImpl

562d470


          remove non used field

59833fb


          remove duplicate javadocs

4750cc4


          extract MaterializedViewsRegistry interface and DummyMaterializedView…

1167a18

…sRegistry to a separate file


          synchronized access to MaterializedViewsRegistry singleton object

828729b


          javadocs

ce1a4c0


          remove invalid comment

4c5deeb


          reduce access of Loader

8a2e419


          javadocs

a330c96


          rename MaterializedViewObjects to MaterializedViewObjectsProvider

2f513cc


          init HiveMaterializedViewsRegistry

bb97da0


          Create MaterializedViewsRegistry instance at field declaration becaus…

9fd7738

…e unit tests does not call the init method.

kasakrisz force-pushed the HIVE-25900-master-mv-reg-refresh branch from 733f011 to 9fd7738 Compare

February 2, 2022 08:19

kgyrtkirk added tests pending tests passed and removed tests failed tests pending labels

github-actions bot commented Apr 4, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Feel free to reach out on the dev@hive.apache.org list if the patch is in need of reviews.

github-actions bot added the stale label

github-actions bot closed this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment