[GitHub] drill pull request: DRILL-4596: Drill should do version check amon...

[jacques-n]

Github user jacques-n commented on the pull request:

https://github.com/apache/drill/pull/474#issuecomment-209081311
  
In general, I'm against version number checking. We did that in the code 
early on but we should be moving towards a capabilities flag approach.

Also agree with Paul in his mention of DRILL-4286, don't think worrying 
about rolling upgrade makes sense until we resolve the issues around 
decommissioning.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-4596: Drill should do version check amon...

[paul-rogers]

Github user paul-rogers commented on the pull request:

https://github.com/apache/drill/pull/474#issuecomment-208959253
  
Some other design issues. The idea of a rollling upgrade presupposes that 
we can shut down a Drillbit, bring up a new one, and the cluster keeps running. 
But, today, bringing down a Drillbit causes all in-flight queries on that node 
to fail. There is no way to mark a node as "quiescent" (up, but not accepting 
new work.) So, a rolling upgrade today would entail a long series of query 
failures as we replace each of, say, 20 or 50 nodes. So, in fact, it is less 
disruptive to take the cluster down, push an upgrade, and bring it back up. 
(See DRILL-4286.)

Back on testing: testing is essential. A feature that allow +/-1 feature 
compatibility is not helpful unless someone (other than the user) can certify 
that it works. If the user gets to do the checking, then it is not very 
helpful: safer just to do a full upgrade.

To emphasize an earlier point: there are two distinct issues. One is a 
managed cluster upgrade (the admin can do it with the help of a management 
tool.) The other are the many Drill clients spread across desktops: that is a 
classic desktop software upgrade. Some might be on planes, others locked in 
desks while someone is on vacation. Let's think about how to upgrade JDBC 
drivers and the like given this reality.

Is the compatiblity policy number or time based? As an admin, can I expect 
to have a three-month window for upgrades? Or, will it sometimes be one month, 
others four months, depending on who changes what? Should we have a time-based 
policy?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-4596: Drill should do version check amon...

[paul-rogers]

Github user paul-rogers commented on the pull request:

https://github.com/apache/drill/pull/474#issuecomment-208943460
  
Hi All. Version compatibility is a complex issue. Do we have a design 
document that explains our goals and policy? Is the goal to allow rolling 
updates of clients? (Drill server at, say 1.8, rolling upgrade of client from 
1.7 to 1.8, old clients work)? Or, is it to allow rolling upgrades of servers? 
Both?

MapR customers receive even releases: 1.4, 1.6, 1.8. Would the +/-1 policy 
benefit them?

As I understand it, each Drill bit is to work with another of +/-1 version. 
But, what I bring up Drill 1.6, Drill 1.5 and Drill 1.4. The 1.5 is happy to 
work with both 1.6 and 1.4. But, the 1.6 and 1.4 versions will fail only when 
they communicate with one another. When will this communication occur? At 
startup? Or, only later when, say, 1.6 tries to send a query to 1.4?

Does this mean that Drillbits should advertise their version in ZooKeeper 
so that we fail fast and can provide a clear error message?

Dremio proposes a new 2.0 release that breaks compatibility. Will Drill 1.9 
(say) be compatible with the incompatile Drill 2.0? Should it be?

As others have said, we need to consider wire protocol and semantics. The 
usual solution is protocol negotiation. If a 1.6 client connects to a 1.7 
server, they agree to "speak" 1.6. If a 1.7 client connects to a 1.6 server, 
they also agree to "speak" 1.6. Such as solution has impact on our messaging 
layer. It increases testing requirements. 

Drill-on-YARN will provide another way to do server upgrades (ramp up a new 
cluster while ramping down an old one.) Otherwise, YARN will need some way to 
run the same cluster, replacing version X drillbits with version X+1 (while 
still running the version X Application Master).

Are all these issues spelled out in a design doc?

IMHO: let's not try to bug fix our way to success here; let's step back and 
work out a complete design.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-4596: Drill should do version check amon...

[arina-ielchiieva]

Github user arina-ielchiieva commented on a diff in the pull request:

https://github.com/apache/drill/pull/474#discussion_r59332891
  
--- Diff: 
common/src/main/java/org/apache/drill/common/util/DrillVersionInfo.java ---
@@ -49,10 +53,52 @@ public static String getVersion() {
 }
   }
 } catch (IOException except) {
-  appVersion = "Unknown";
+  appVersion = UNKNOWN_VERSION;
 }
 return appVersion;
   }
 
+  /**
+   * Compare two Drill versions disregarding build number and comparing 
only major and minor versions.
+   * Versions are considered to be compatible:
+   * 1. if current version is the same as version to compare.
+   * 2. if current version minor version + 1 is the same as version to 
compare.
+   */
+  public static boolean isVersionsCompatible(String currentVersion, String 
versionToCompare) {
+if (currentVersion != null && currentVersion.equals(versionToCompare)) 
{
+  return true;
+}
+
+BigDecimal currentVersionDecimal = getVersionAsDecimal(currentVersion);
+BigDecimal versionToCompareDecimal = 
getVersionAsDecimal(versionToCompare);
+
+if (currentVersionDecimal != null && versionToCompareDecimal != null) {
--- End diff --

I guess, yes. Ideally all drillbits in cluster will be with the same 
version, only during rolling upgrades this situation may occur.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-4596: Drill should do version check amon...

[yufeldman]

Github user yufeldman commented on a diff in the pull request:

https://github.com/apache/drill/pull/474#discussion_r59297387
  
--- Diff: 
common/src/main/java/org/apache/drill/common/util/DrillVersionInfo.java ---
@@ -49,10 +53,52 @@ public static String getVersion() {
 }
   }
 } catch (IOException except) {
-  appVersion = "Unknown";
+  appVersion = UNKNOWN_VERSION;
 }
 return appVersion;
   }
 
+  /**
+   * Compare two Drill versions disregarding build number and comparing 
only major and minor versions.
+   * Versions are considered to be compatible:
+   * 1. if current version is the same as version to compare.
+   * 2. if current version minor version + 1 is the same as version to 
compare.
+   */
+  public static boolean isVersionsCompatible(String currentVersion, String 
versionToCompare) {
+if (currentVersion != null && currentVersion.equals(versionToCompare)) 
{
+  return true;
+}
+
+BigDecimal currentVersionDecimal = getVersionAsDecimal(currentVersion);
+BigDecimal versionToCompareDecimal = 
getVersionAsDecimal(versionToCompare);
+
+if (currentVersionDecimal != null && versionToCompareDecimal != null) {
--- End diff --

So for Drillbit with version 1.6 Drillbit with version 1.7 will be 
included, while for a Drillbit with version 1.7 Drillbit with version 1.6 will 
not be included?  Does it mean that at any particular time a particular 
Drillbit will have different view on the cluster from some other Drillbit?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-4596: Drill should do version check amon...

[arina-ielchiieva]

Github user arina-ielchiieva commented on the pull request:

https://github.com/apache/drill/pull/474#issuecomment-208557847
  
If we add new type to version 1.7, it can't work with  version 1.6 where 
this type is absent, that's why I am adding support only for +1 version. At 
this point in my implementation, 1.6 can work with 1.6 and 1.7. 1.7 can work 
with 1.7 and 1.8.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-4596: Drill should do version check amon...

[yufeldman]

Github user yufeldman commented on the pull request:

https://github.com/apache/drill/pull/474#issuecomment-208554692
  
Not sure how that comment contradicts with what I said. Adding new feature 
in the next version should not break previous version. 

> On Apr 11, 2016, at 12:41 PM, arina-ielchiieva  
wrote:
> 
> I thought it's hard to maintain backward compatibility. Quoting Parth 
comment from Jira:
> "Drill needs to be able to run queries that depend only on features that 
are in the oldest version of Drill in the cluster. Say for example, most 
drillbits are at version N and some are at version N+1. You added support for a 
new type foo in version N+1. Any queries not using or depending on type foo 
should run successfully. Queries depending on foo can only run successfully 
after all the drillbits are upgraded to N+1."
> 
> —
> You are receiving this because you commented.
> Reply to this email directly or view it on GitHub
> 



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-4596: Drill should do version check amon...

[arina-ielchiieva]

Github user arina-ielchiieva commented on the pull request:

https://github.com/apache/drill/pull/474#issuecomment-208521519
  
I thought it's hard to maintain backward compatibility. Quoting Parth 
comment from Jira:
"Drill needs to be able to run queries that depend only on features that 
are in the oldest version of Drill in the cluster. Say for example, most 
drillbits are at version N and some are at version N+1. You added support for a 
new type foo in version N+1. Any queries not using or depending on type foo 
should run successfully. Queries depending on foo can only run successfully 
after all the drillbits are upgraded to N+1."


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-4596: Drill should do version check amon...

[yufeldman]

Github user yufeldman commented on the pull request:

https://github.com/apache/drill/pull/474#issuecomment-208434359
  
Should it be +-1, not just +1 ? Otherwise in your scheme 1.9 can work with
2.0, but can not with 1.8, while 1.8 can work with 1.9.

On Mon, Apr 11, 2016 at 9:08 AM, arina-ielchiieva 
wrote:

> Updated PR.
> New implementation approach:
> 1. we allow to register any drillbit (disregarding the version).
> 2. instead of getting available endpoints from coordinator, we'll be
> receiving compatible endpoints. Version compatibility will be defined 
using
> main application version (major.minor (+ 1). For example, drillbit with
> version 1.8 can work with 1.8 and 1.9. Drillbit with version 1.9 can work
> with 1.9 and 2.0 and so on. Even during rolling upgrades, compatible
> endpoints will be able to work together.
>
> —
> You are receiving this because you commented.
> Reply to this email directly or view it on GitHub
> 
>



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-4596: Drill should do version check amon...

[arina-ielchiieva]

Github user arina-ielchiieva commented on the pull request:

https://github.com/apache/drill/pull/474#issuecomment-208428822
  
Updated PR.
New implementation approach:
1. we allow to register any drillbit (disregarding the version).
2. instead of getting available endpoints from coordinator, we'll be 
receiving compatible endpoints. Version compatibility will be defined using 
main application version (major.minor (+ 1). For example, drillbit with version 
1.8 can work with 1.8 and 1.9. Drillbit with version 1.9 can work with 1.9 and 
2.0 and so on. Even during rolling upgrades, compatible endpoints will be able 
to work together.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-4596: Drill should do version check amon...

[arina-ielchiieva]

Github user arina-ielchiieva commented on a diff in the pull request:

https://github.com/apache/drill/pull/474#discussion_r59077998
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/Drillbit.java ---
@@ -207,6 +209,28 @@ private void javaPropertiesToSystemOptions() {
   }
 
   /**
+   * Disallow registering drillbit when:
+   * 1. version is unknown;
+   * 2. drillbit with different version has been already registered.
+   */
+  private void checkVersion(DrillbitEndpoint endpoint) throws 
DrillbitStartupException {
+String currentVersion = endpoint.getVersion();
+if (DrillVersionInfo.UNKNOWN_VERSION.equals(currentVersion)) {
+  throw new DrillbitStartupException("Drillbit version is unknown.");
+}
+
+for (DrillbitEndpoint registeredEnpoint : 
coord.getAvailableEndpoints()) {
+  if (!currentVersion.equals(registeredEnpoint.getVersion())) {
--- End diff --

Agree. Probably it's better to compare each module major versions.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-4596: Drill should do version check amon...

[sudheeshkatkam]

Github user sudheeshkatkam commented on a diff in the pull request:

https://github.com/apache/drill/pull/474#discussion_r59065316
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/Drillbit.java ---
@@ -207,6 +209,28 @@ private void javaPropertiesToSystemOptions() {
   }
 
   /**
+   * Disallow registering drillbit when:
+   * 1. version is unknown;
+   * 2. drillbit with different version has been already registered.
+   */
+  private void checkVersion(DrillbitEndpoint endpoint) throws 
DrillbitStartupException {
+String currentVersion = endpoint.getVersion();
+if (DrillVersionInfo.UNKNOWN_VERSION.equals(currentVersion)) {
+  throw new DrillbitStartupException("Drillbit version is unknown.");
+}
+
+for (DrillbitEndpoint registeredEnpoint : 
coord.getAvailableEndpoints()) {
+  if (!currentVersion.equals(registeredEnpoint.getVersion())) {
--- End diff --

How does this check work with regards to [protobuf backwards 
compatibility](https://developers.google.com/protocol-buffers/docs/javatutorial#extending-a-protocol-buffer)?
 Endpoints currently do not have a version. So if two bits (A and B), one with 
and one without this change, are started, the two start order cases (A after B, 
and B after A) should pass.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-4596: Drill should do version check amon...

[JohnOmernik]

Github user JohnOmernik commented on a diff in the pull request:

https://github.com/apache/drill/pull/474#discussion_r59064857
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/Drillbit.java ---
@@ -207,6 +209,28 @@ private void javaPropertiesToSystemOptions() {
   }
 
   /**
+   * Disallow registering drillbit when:
+   * 1. version is unknown;
+   * 2. drillbit with different version has been already registered.
--- End diff --

Agreed.  In addition to potential soft enforcement, I think we should 
consider the differences of a version defined in a pom.xml, the version of the 
running drillbit, and the version of a drill cluster.  By using a setting in 
drill-override to specify what the drill cluster version is, we don't get 
accident "first bit to register is the wrong version" situations.  Are there 
situations where we might want to allow a specific Drill version + all minor 
versions at that level, or Drill version + 1 Major version for rolling upgrades?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-4596: Drill should do version check amon...

[yufeldman]

Github user yufeldman commented on a diff in the pull request:

https://github.com/apache/drill/pull/474#discussion_r59063793
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/Drillbit.java ---
@@ -207,6 +209,28 @@ private void javaPropertiesToSystemOptions() {
   }
 
   /**
+   * Disallow registering drillbit when:
+   * 1. version is unknown;
+   * 2. drillbit with different version has been already registered.
--- End diff --

What if you first DrillBit registered had "bad" version? Somebody would 
have to bring it down, so others can start?
Should it be (at least) option to have soft enforcement, rather than just 
hard enforcement.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-4596: Drill should do version check amon...

[arina-ielchiieva]

Github user arina-ielchiieva commented on the pull request:

https://github.com/apache/drill/pull/474#issuecomment-207529907
  
Drill version is defined from build manifest file [1].
There is no config setting to override it. Though if you want to change 
version in "master", you'll have to update version property in pom.xml files. 
(1.7.0-SNAPSHOT)

[1] 
https://github.com/apache/drill/blob/master/common/src/main/java/org/apache/drill/common/util/DrillVersionInfo.java



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-4596: Drill should do version check amon...

[JohnOmernik]

Github user JohnOmernik commented on the pull request:

https://github.com/apache/drill/pull/474#issuecomment-207520784
  
So how is the version actually set? Is it just the bit to register with 
Drill? Is there a config setting that we can set in drill-override that sets 
the cluster version?  Where is the "master" version stored, and how do we move 
away from that? 

Thanks


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-4596: Drill should do version check amon...

[arina-ielchiieva]

GitHub user arina-ielchiieva opened a pull request:

https://github.com/apache/drill/pull/474

DRILL-4596: Drill should do version check among drillbits



You can merge this pull request into a Git repository by running:

$ git pull https://github.com/arina-ielchiieva/drill DRILL-4596

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/drill/pull/474.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 #474


commit 76e857c29becee22daf116dd58a36fe34a920595
Author: Arina Ielchiieva 
Date:   2016-04-08T14:22:47Z

DRILL-4596: Drill should do version check among drillbits




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---