[GitHub] storm pull request #2761: STORM-2947: Remove deprecated field and method fro...
Github user asfgit closed the pull request at: https://github.com/apache/storm/pull/2761 ---
[GitHub] storm pull request #2761: STORM-2947: Remove deprecated field and method fro...
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2761#discussion_r205720665
--- Diff:
storm-client/src/jvm/org/apache/storm/generated/ClusterSummary.java ---
@@ -63,8 +60,6 @@ public static _Fields findByThriftId(int fieldId) {
switch(fieldId) {
case 1: // SUPERVISORS
return SUPERVISORS;
-case 2: // NIMBUS_UPTIME_SECS
- return NIMBUS_UPTIME_SECS;
case 3: // TOPOLOGIES
return TOPOLOGIES;
--- End diff --
This is generated code. The comment about this is here
https://github.com/apache/storm/pull/2761/files#diff-61bbff6c872f2afa10a3b695c1cfe4a3R209
---
[GitHub] storm pull request #2761: STORM-2947: Remove deprecated field and method fro...
Github user danny0405 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2761#discussion_r205702075
--- Diff:
storm-client/src/jvm/org/apache/storm/generated/ClusterSummary.java ---
@@ -63,8 +60,6 @@ public static _Fields findByThriftId(int fieldId) {
switch(fieldId) {
case 1: // SUPERVISORS
return SUPERVISORS;
-case 2: // NIMBUS_UPTIME_SECS
- return NIMBUS_UPTIME_SECS;
case 3: // TOPOLOGIES
return TOPOLOGIES;
--- End diff --
Ok, got your idea, but this will make the code a little incoherent, add a
comment here will make sense.
---
[GitHub] storm pull request #2761: STORM-2947: Remove deprecated field and method fro...
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2761#discussion_r203693962
--- Diff:
storm-client/src/jvm/org/apache/storm/generated/ClusterSummary.java ---
@@ -63,8 +60,6 @@ public static _Fields findByThriftId(int fieldId) {
switch(fieldId) {
case 1: // SUPERVISORS
return SUPERVISORS;
-case 2: // NIMBUS_UPTIME_SECS
- return NIMBUS_UPTIME_SECS;
case 3: // TOPOLOGIES
return TOPOLOGIES;
--- End diff --
It's my impression that if we don't keep the field ids as they are now, it
won't be possible for people to do a rolling upgrade. E.g. if we changed
TOPOLOGIES to have id 2, then someone doing a rolling upgrade might have a
machine send ClusterSummary with TOPOLOGIES id 3 to an upgraded machine where
TOPOLOGIES has id 2, which breaks deserialization.
---
[GitHub] storm pull request #2761: STORM-2947: Remove deprecated field and method fro...
Github user danny0405 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2761#discussion_r203651733
--- Diff:
storm-client/src/jvm/org/apache/storm/generated/ClusterSummary.java ---
@@ -63,8 +60,6 @@ public static _Fields findByThriftId(int fieldId) {
switch(fieldId) {
case 1: // SUPERVISORS
return SUPERVISORS;
-case 2: // NIMBUS_UPTIME_SECS
- return NIMBUS_UPTIME_SECS;
case 3: // TOPOLOGIES
return TOPOLOGIES;
--- End diff --
Should we keep the sequence number as it is now ?
---
[GitHub] storm pull request #2761: STORM-2947: Remove deprecated field and method fro...
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2761#discussion_r202126575
--- Diff: storm-client/src/storm.thrift ---
@@ -206,10 +206,8 @@ struct NimbusSummary {
struct ClusterSummary {
1: required list supervisors;
- //@deprecated, please use nimbuses.uptime_secs instead.
- 2: optional i32 nimbus_uptime_secs = 0;
- 3: required list topologies;
- 4: required list nimbuses;
+ 2: required list topologies;
+ 3: required list nimbuses;
--- End diff --
Thanks. Is it worth it to link to in DEVELOPER,md, or somewhere else do you
think?
---
[GitHub] storm pull request #2761: STORM-2947: Remove deprecated field and method fro...
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2761#discussion_r202115684
--- Diff: storm-client/src/storm.thrift ---
@@ -206,10 +206,8 @@ struct NimbusSummary {
struct ClusterSummary {
1: required list supervisors;
- //@deprecated, please use nimbuses.uptime_secs instead.
- 2: optional i32 nimbus_uptime_secs = 0;
- 3: required list topologies;
- 4: required list nimbuses;
+ 2: required list topologies;
+ 3: required list nimbuses;
--- End diff --
https://diwakergupta.github.io/thrift-missing-guide/ describes some of this.
Specifically look at the bottom part about Versioning/Compatibility.
But there is some of it spread throughout the doc.
---
[GitHub] storm pull request #2761: STORM-2947: Remove deprecated field and method fro...
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2761#discussion_r202086557
--- Diff: storm-client/src/storm.thrift ---
@@ -206,10 +206,8 @@ struct NimbusSummary {
struct ClusterSummary {
1: required list supervisors;
- //@deprecated, please use nimbuses.uptime_secs instead.
- 2: optional i32 nimbus_uptime_secs = 0;
- 3: required list topologies;
- 4: required list nimbuses;
+ 2: required list topologies;
+ 3: required list nimbuses;
--- End diff --
Sure. Do you have a link to the guidelines, I didn't see anything in
DEVELOPER.md?
---
[GitHub] storm pull request #2761: STORM-2947: Remove deprecated field and method fro...
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2761#discussion_r202070445
--- Diff: storm-client/src/storm.thrift ---
@@ -206,10 +206,8 @@ struct NimbusSummary {
struct ClusterSummary {
1: required list supervisors;
- //@deprecated, please use nimbuses.uptime_secs instead.
- 2: optional i32 nimbus_uptime_secs = 0;
- 3: required list topologies;
- 4: required list nimbuses;
+ 2: required list topologies;
+ 3: required list nimbuses;
--- End diff --
Can we please follow the guidelines for maintaining wire compatibility?
this way old clients still have a chance of working with the newer servers.
```
struct ClusterSummary {
1: required list supervisors;
// removed (DO NOT REUSE) 2:
3: required list topologies;
4: required list nimbuses;
}
```
---
[GitHub] storm pull request #2761: STORM-2947: Remove deprecated field and method fro...
GitHub user srdo opened a pull request: https://github.com/apache/storm/pull/2761 STORM-2947: Remove deprecated field and method from Thrift Part of https://issues.apache.org/jira/browse/STORM-2947 You can merge this pull request into a Git repository by running: $ git pull https://github.com/srdo/storm STORM-2947-thrift Alternatively you can review and apply these changes as the patch at: https://github.com/apache/storm/pull/2761.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 #2761 commit 1768062f7b5f069a5a1b6a55218e6195f5e8f66a Author: Stig Rohde Døssing Date: 2018-07-10T13:56:34Z STORM-2947: Remove deprecated field and method from Thrift ---
