[GitHub] drill issue #1014: DRILL-5771: Fix serDe errors for format plugins
Github user arina-ielchiieva commented on the issue: https://github.com/apache/drill/pull/1014 @parthchandra done. ---
[GitHub] drill issue #1014: DRILL-5771: Fix serDe errors for format plugins
Github user parthchandra commented on the issue: https://github.com/apache/drill/pull/1014 @arina-ielchiieva can you resolve the merge conflict? ---
[GitHub] drill issue #1014: DRILL-5771: Fix serDe errors for format plugins
Github user ilooner commented on the issue: https://github.com/apache/drill/pull/1014 Thanks I took a look. ---
[GitHub] drill issue #1014: DRILL-5771: Fix serDe errors for format plugins
Github user arina-ielchiieva commented on the issue: https://github.com/apache/drill/pull/1014 @ilooner Paul has correctly described race condition issue. At planning time when we have defined plugin configuration we want to make sure it would stay the same during query execution. Relying on plugin configuration name is unsafe since config under this particular name can change and it can be that one fragment of the query is executed with one configuration and another with different (typical race condition issue). Regarding your last question, during physical plan creation I don't think we deserialize DrillTable, we rather deserialize query fragments (it can be seen from json profile). Also you can start looking, for example, from here: https://github.com/apache/drill/blob/master/exec/java-exec/src/main/java/org/apache/drill/exec/work/QueryWorkUnit.java#L59 ---
[GitHub] drill issue #1014: DRILL-5771: Fix serDe errors for format plugins
Github user ilooner commented on the issue: https://github.com/apache/drill/pull/1014 @paul-rogers Thanks for the explanation. Your explanation is in sync with Arina's descriptions in the ticket and with the code changes. The only point of confusion I have now is with regards to the Calcite things changes you mentioned. I was under the impression that the following happened: 1. FormatPlugins created a DrillTable, which held the format configuration. 1. The DrillTable is then encapsulated in a ScanOperator 1. The ScanOperator automagically gets serialized into the PhysicalPlan with the DrillTable and corresponding format configuration correctly. But it seems like there are more Calcite things at work here. @arina-ielchiieva could you please point me to the general flow I need to look at in order to understand how the FormatPluginConfig interacts with Calcite and gets serialized into the PhysicalPlan? ---
[GitHub] drill issue #1014: DRILL-5771: Fix serDe errors for format plugins
Github user paul-rogers commented on the issue: https://github.com/apache/drill/pull/1014 @ilooner, there are two phases of issues. The problem is in the planner, but we've been talking about serialization out to the workers. Here, we can learn from the work Arina did with dynamic UDFs, especially in the synchronization aspects as it has potential challenges similar to what we have here. Let's walk through the lifecycle. * User A defines a storage plugin config, call it P. * User B else runs a query Q that references P. * At the same time, user A changes P to P'. * Query Q is distributed to nodes. * At the same time, user A changes P again to produce P''. This is a classic distributed system synchronization problem. As we discussed, once the query is planned, the contents of the plugin definition are serialized as part of the physical plan for each fragment. As a result we "freeze" the contents of P at the time of serialization. The problem that Arina seems to be describing is during planning. Rather than taking a copy of P at some fixed point in time, then always using that copy; we seem to be holding a named reference to P. This introduces the obvious synchronization issues. So, what we should do is, once a query resolves P, the query takes a copy and uses that copy for the rest of query planning and execution. This resolves race conditions by saying that a query (on all nodes) uses the version of the plugin definition that existed at the moment that the reference to the plugin definition was first resolved by that query. The query will be oblivious to all subsequent updates. Since plugin definition is done as part of workspace and table definition in Calcite, this is likely to be a tricky change; it is not clear that Calcite offers a per-query context to hold such information. Arina probably can offer advice on this front. ---
[GitHub] drill issue #1014: DRILL-5771: Fix serDe errors for format plugins
Github user ilooner commented on the issue: https://github.com/apache/drill/pull/1014 @paul-rogers I follow your explanation except for the last paragraph. The Drill UI let's me update storage plugins and their formatters at runtime (at least on my laptop) by going to **Storage** tab and clicking on **Update** for a plugin. It also let's me add storage plugins provided the necessary jars are in the classpath. I thought the goal of this change was to address the fact that such a runtime update cannot be propagated to the cluster atomically, so it is unreliable to reference a FormatPluginConfig by name (which was done previously). It is, however, safe to copy the entire FormatPluginConfig into the query plan as you said. Is this accurate? ---
[GitHub] drill issue #1014: DRILL-5771: Fix serDe errors for format plugins
Github user paul-rogers commented on the issue: https://github.com/apache/drill/pull/1014 Not sure the description here is entirely correct. Let's separate two concepts: the plugin (code) and the plugin definition (the stuff in JSON.) Plugin definitions are stored in ZK and retrieved by the Foreman. There may be some form of race condition in the Foreman, but that's not my focus here. The plugin *definition* is read by the Forman and serialized into the physical plan. Each worker reads the definition from the physical plan. For this reason, the worker's definition can never be out of date: it is the definition used when serializing the plan. Further, Drill allows table functions which provide query-time name/value pair settings for format plugin properties. The only way these can work is to be serialized along with the query. So, the actual serialized format plugin definition, included with the query, includes both the ZK information and the table function information. ---
[GitHub] drill issue #1014: DRILL-5771: Fix serDe errors for format plugins
Github user ilooner commented on the issue: https://github.com/apache/drill/pull/1014 @arina-ielchiieva - The parts addressing DRILL-4640 and DRILL-5166 LGTM - I think the fix for DRILL-5771 LGTM but I would like write down what I think is happening and confirm with you that my understanding is correct. This is mostly just a learning exercise for me since I am not very familiar with this part of the code :). In DRILL-5771 there were two issues. ## Race Conditions With Format Plugins ### Issue The following used to happen before the fix: 1. When using an existing format plugin, the **FormatPlugin** would create a **DrillTable** with a **NamedFormatPluginConfig** which only contains the name of the format plugin to use. 1. The **ScanOperator** created for a **DrillTable** will contain the **NamedFormatPluginConfig** 1. When the **ScanOperators** are serialized in to the physical plan the serialized **ScanOperator** will only contain the name of the format plugin to use. 1. When a worker deserializes the physical plan to do a scan, he gets the name of the **FormatPluginConfig** to use. 1. The worker then looks up the correct **FormatPlugin** in the **FormatCreator** using the name he has. 1. The worker can get into trouble if the **FormatPlugins** he has cached in his **FormatCreator** is out of sync with the rest of the cluster. ### Fix Race conditions are eliminated because the **DrillTables** returned by the **FormatPlugins** no longer contain a **NamedFormatPluginConfig**, they contain the full **FormatPluginConfig** not just a name alias. So when a query is executed: 1. The ScanOperator contains the complete **FormatPluginConfig** 1. When the physical plan is serialized it contains the complete **FormatPluginConfig** for each scan operator. 1. When a worker node deserializes the ScanOperator it also has the complete **FormatPluginConfig** so it can reconstruct the **FormatPlugin** correctly, whereas previously the worker would have to do a lookup using the **FormatPlugin** name in the **FormatCreator** when the cache in the **FormatCreator** may be out of sync with the rest of the cluster. ## FormatPluginConfig Equals and HashCode ### Issue The **FileSystemPlugin** looks up **FormatPlugins** corresponding to a **FormatPluginConfig** in formatPluginsByConfig. However, the **FormatPluginConfig** implementations didn't override equals and hashCode. ---
[GitHub] drill issue #1014: DRILL-5771: Fix serDe errors for format plugins
Github user priteshm commented on the issue: https://github.com/apache/drill/pull/1014 @ilooner can you please review this? ---
