You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by ilooner <gi...@git.apache.org> on 2017/09/23 07:28:58 UTC
[GitHub] drill pull request #957: DRILL-5809 Fixed Option Storage Backward Compatibil...
GitHub user ilooner opened a pull request:
https://github.com/apache/drill/pull/957
DRILL-5809 Fixed Option Storage Backward Compatibility Issue Introduced With Option System Change
This PR resolves the backward compatibility issue introduced with https://github.com/apache/drill/commit/6adeb986016a769755fd5e8fc66244ee1e8d18e1 using the strategy outlined by Paul in this thread http://mail-archives.apache.org/mod_mbox/drill-dev/201709.mbox/%3CBLUPR16MB0371144903DC73E0505BA1FEC3670%40BLUPR16MB0371.namprd16.prod.outlook.com%3E
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ilooner/drill DRILL-5809
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/drill/pull/957.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #957
----
commit bda878ae24410b68b59291e65fec8de17f777efe
Author: Timothy Farkas <ti...@apache.org>
Date: 2017-09-22T05:40:46Z
- Fixed backward compatability issues with zookeeper
commit 6622e7320b196b747cc8e05dfc53d8f4163d5b98
Author: Timothy Farkas <ti...@apache.org>
Date: 2017-09-22T18:26:56Z
- Added zookeeper backwards compatibility test
commit 6eab7f5333c58594143bbf73bf86489c852af040
Author: Timothy Farkas <ti...@apache.org>
Date: 2017-09-22T20:43:38Z
- Only persisting required information in zookeeper
commit 8d5f181e2c30b5c47fee74bee80dd2e8af1531a8
Author: Timothy Farkas <ti...@apache.org>
Date: 2017-09-23T01:17:32Z
- Got unit tests passing
commit 5222f74afc8a3e94af026877663a826e844cd29c
Author: Timothy Farkas <ti...@apache.org>
Date: 2017-09-23T05:40:30Z
- Unit Test Cleanup
- Fixed issue in BaseTestQuery where it can connect to the wrong Drillbit
- Fixed bug in SystemOptionManager
commit e60e52b93032797de5a7a592b11dd4819e8dee01
Author: Timothy Farkas <ti...@apache.org>
Date: 2017-09-23T06:43:27Z
- Attempt to fix jenkins bug
----
---
[GitHub] drill pull request #957: DRILL-5809 Fixed Option Storage Backward Compatibil...
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/957#discussion_r140644789
--- Diff: common/src/test/java/org/apache/drill/testutils/DirTestWatcher.java ---
@@ -0,0 +1,56 @@
+/**
+ * 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.drill.testutils;
+
+import org.apache.commons.io.FileUtils;
+import org.junit.rules.TestWatcher;
+import org.junit.runner.Description;
+
+import java.io.File;
+
+public class DirTestWatcher extends TestWatcher {
--- End diff --
Perhaps a bit of Javadoc to say what this does?
---
[GitHub] drill pull request #957: DRILL-5809 Fixed Option Storage Backward Compatibil...
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/957#discussion_r140936230
--- Diff: common/src/test/java/org/apache/drill/testutils/DirTestWatcher.java ---
@@ -0,0 +1,61 @@
+/**
+ * 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.drill.testutils;
+
+import org.apache.commons.io.FileUtils;
+import org.junit.rules.TestWatcher;
+import org.junit.runner.Description;
+
+import java.io.File;
+
+/*
+ * This JUnit {@link TestWatcher} creates a unique directory for each JUnit test in the project's
+ * target folder at the start of each test. This directory can be used as a temporary directory to store
+ * files for the test. The directory and its contents are deleted at the end of the test.
--- End diff --
Thanks for the explanation!
Many existing tests use Java temporary directories. Not now, but over time, would be good to migrate that code to use this class. `ClusterFixture` and/or `ClientFixture` do something like this. I believe that `BaseTestQuery` does also (since that's where I copied the code from...)
---
[GitHub] drill pull request #957: DRILL-5809 Fixed Option Storage Backward Compatibil...
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/957#discussion_r140936599
--- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/server/options/PersistedOptionValueTest.java ---
@@ -0,0 +1,226 @@
+/**
+ * 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.drill.exec.server.options;
+
+import com.fasterxml.jackson.annotation.JsonInclude;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.apache.drill.common.util.FileUtils;
+import org.apache.drill.exec.serialization.JacksonSerializer;
+import org.apache.drill.exec.store.sys.PersistentStoreConfig;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.io.IOException;
+
+public class PersistedOptionValueTest {
+ /**
+ * DRILL-5809
+ * Note: If this test breaks you are probably breaking backward and forward compatibility. Verify with the community
--- End diff --
Great comment!
---
[GitHub] drill pull request #957: DRILL-5809 Fixed Option Storage Backward Compatibil...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/drill/pull/957
---
[GitHub] drill pull request #957: DRILL-5809 Fixed Option Storage Backward Compatibil...
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/957#discussion_r140936539
--- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/server/options/OptionValueTest.java ---
@@ -0,0 +1,72 @@
+/**
--- End diff --
/*
---
[GitHub] drill pull request #957: DRILL-5809 Fixed Option Storage Backward Compatibil...
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/957#discussion_r140936558
--- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/server/options/PersistedOptionValueTest.java ---
@@ -0,0 +1,226 @@
+/**
--- End diff --
/*
---
[GitHub] drill pull request #957: DRILL-5809 Fixed Option Storage Backward Compatibil...
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/957#discussion_r140936016
--- Diff: common/src/test/java/org/apache/drill/testutils/DirTestWatcher.java ---
@@ -0,0 +1,61 @@
+/**
+ * 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.drill.testutils;
+
+import org.apache.commons.io.FileUtils;
+import org.junit.rules.TestWatcher;
+import org.junit.runner.Description;
+
+import java.io.File;
+
+/*
--- End diff --
This is Javadoc, so please use /** comments here.
---
[GitHub] drill issue #957: DRILL-5809 Fixed Option Storage Backward Compatibility Iss...
Posted by ilooner <gi...@git.apache.org>.
Github user ilooner commented on the issue:
https://github.com/apache/drill/pull/957
@paul-rogers Applied comments and did a few other changes. Unit tests cover backward compatability. I manually tested forward compatability locally by saving some options with the new version using the LocalStore. Then I rolled back to an older version of Drill and loaded the data from the LocalStore.
---
[GitHub] drill pull request #957: DRILL-5809 Fixed Option Storage Backward Compatibil...
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/957#discussion_r140936258
--- Diff: common/src/test/java/org/apache/drill/testutils/DirTestWatcher.java ---
@@ -0,0 +1,61 @@
+/**
+ * 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.drill.testutils;
+
+import org.apache.commons.io.FileUtils;
+import org.junit.rules.TestWatcher;
+import org.junit.runner.Description;
+
+import java.io.File;
+
+/*
+ * This JUnit {@link TestWatcher} creates a unique directory for each JUnit test in the project's
+ * target folder at the start of each test. This directory can be used as a temporary directory to store
+ * files for the test. The directory and its contents are deleted at the end of the test.
+ */
+public class DirTestWatcher extends TestWatcher {
+ private String dirPath;
+ private File dir;
--- End diff --
final?
---
[GitHub] drill issue #957: DRILL-5809 Fixed Option Storage Backward Compatibility Iss...
Posted by ilooner <gi...@git.apache.org>.
Github user ilooner commented on the issue:
https://github.com/apache/drill/pull/957
@paul-rogers Applied comments and squashed commits
---
[GitHub] drill pull request #957: DRILL-5809 Fixed Option Storage Backward Compatibil...
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/957#discussion_r140644808
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/options/OptionValue.java ---
@@ -44,6 +44,14 @@
*/
@JsonInclude(Include.NON_NULL)
public class OptionValue implements Comparable<OptionValue> {
+ public static final String JSON_KIND = "kind";
+ public static final String JSON_ACCESSIBLESCOPES = "accessibleScopes";
--- End diff --
`JSON_ACCESSIBLESCOPES` --> `JSON_ACCESSIBLE_SCOPES`?
---
[GitHub] drill pull request #957: DRILL-5809 Fixed Option Storage Backward Compatibil...
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/957#discussion_r140935951
--- Diff: common/src/test/java/org/apache/drill/testutils/DirTestWatcher.java ---
@@ -0,0 +1,61 @@
+/**
--- End diff --
For copyright, please use regular /* comments, not Javadoc /** comments. That way, the copyright won't be copied into every classes' Javadoc if/when we ever generate Javadoc.
---
[GitHub] drill pull request #957: DRILL-5809 Fixed Option Storage Backward Compatibil...
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/957#discussion_r140936344
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/options/PersistedOptionValue.java ---
@@ -0,0 +1,310 @@
+/**
+ * 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.drill.exec.server.options;
+
+import com.fasterxml.jackson.annotation.JsonIgnore;
+import com.fasterxml.jackson.annotation.JsonInclude;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.core.JsonParser;
+import com.fasterxml.jackson.core.JsonProcessingException;
+import com.fasterxml.jackson.core.ObjectCodec;
+import com.fasterxml.jackson.databind.DeserializationContext;
+import com.fasterxml.jackson.databind.JavaType;
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
+import com.fasterxml.jackson.databind.deser.std.StdDeserializer;
+import com.google.common.base.Preconditions;
+
+import java.io.IOException;
+
+/**
+ * <p>
+ * This represents a persisted {@link OptionValue}. Decoupling the {@link OptionValue} from what
+ * is persisted will prevent us from accidentally breaking backward compatibility in the future
+ * when the {@link OptionValue} changes. Additionally when we do change the format of stored options we
+ * will not have to change much code since this is already designed with backward compatibility in mind.
--- End diff --
Nice!
---
[GitHub] drill issue #957: DRILL-5809 Fixed Option Storage Backward Compatibility Iss...
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on the issue:
https://github.com/apache/drill/pull/957
Thanks for the changes.
+1
---
[GitHub] drill pull request #957: DRILL-5809 Fixed Option Storage Backward Compatibil...
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/957#discussion_r140936303
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/options/PersistedOptionValue.java ---
@@ -0,0 +1,310 @@
+/**
--- End diff --
/*
---
[GitHub] drill pull request #957: DRILL-5809 Fixed Option Storage Backward Compatibil...
Posted by ilooner <gi...@git.apache.org>.
Github user ilooner commented on a diff in the pull request:
https://github.com/apache/drill/pull/957#discussion_r140946604
--- Diff: common/src/test/java/org/apache/drill/testutils/DirTestWatcher.java ---
@@ -0,0 +1,61 @@
+/**
+ * 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.drill.testutils;
+
+import org.apache.commons.io.FileUtils;
+import org.junit.rules.TestWatcher;
+import org.junit.runner.Description;
+
+import java.io.File;
+
+/*
+ * This JUnit {@link TestWatcher} creates a unique directory for each JUnit test in the project's
+ * target folder at the start of each test. This directory can be used as a temporary directory to store
+ * files for the test. The directory and its contents are deleted at the end of the test.
+ */
+public class DirTestWatcher extends TestWatcher {
+ private String dirPath;
+ private File dir;
--- End diff --
Can't be final since it's not set in a constructor. It is also reset for every unit test in a test class.
---
[GitHub] drill issue #957: DRILL-5809 Fixed Option Storage Backward Compatibility Iss...
Posted by ilooner <gi...@git.apache.org>.
Github user ilooner commented on the issue:
https://github.com/apache/drill/pull/957
@paul-rogers
---