Skip to content

[STORM-1932] Change Duration's value data type - #1527

Closed
darionyaphet wants to merge 1 commit into
apache:masterfrom
darionyaphet:STORM-1932
Closed

[STORM-1932] Change Duration's value data type#1527
darionyaphet wants to merge 1 commit into
apache:masterfrom
darionyaphet:STORM-1932

Conversation

@darionyaphet

@darionyaphetdarionyaphet commented Jun 28, 2016

Copy link
Copy Markdown
Contributor

STORM-1932

BaseWindowedBolt Duration value is integer now .

Use long to express time interval maybe better.

@harshach

harshach commented Jul 5, 2016

Copy link
Copy Markdown
Contributor

+1

@HeartSaVioR

HeartSaVioR commented Jul 5, 2016

Copy link
Copy Markdown
Contributor

@darionyaphet
Build is failing with your changes. Could you take care of this?
I'd recommend to build the entire code by mvn clean install and see success at least once, and publish your change.

@darionyaphet

darionyaphet commented Jul 5, 2016

Copy link
Copy Markdown
ContributorAuthor

@HeartSaVioR It seems fixed .

@HeartSaVioR

HeartSaVioR commented Jul 5, 2016

Copy link
Copy Markdown
Contributor

@darionyaphet Great. Thanks for the work. +1

arunmahadevan
arunmahadevan reviewed Jul 6, 2016
View reviewed changes
Comment threadstorm-core/src/jvm/org/apache/storm/topology/WindowedBoltExecutor.java
}

private int getTopologyTimeoutMillis(Map stormConf) {
private long getTopologyTimeoutMillis(Map stormConf) {

@arunmahadevanarunmahadevanJul 6, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TOPOLOGY_MESSAGE_TIMEOUT_SECS is integer. I don't think this method should return a long.

@darionyaphetdarionyaphetJul 14, 2016

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

finally , the return value is timeout * 1000 , it have convert to millisecond . Long is a better choose .

@arunmahadevanarunmahadevanJul 15, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had missed that the value is getting converted to ms. Ok to have a long.

@arunmahadevan

arunmahadevan commented Jul 6, 2016

Copy link
Copy Markdown
Contributor

@darionyaphet You need to make more changes,

  1. you should use longValue() instead of the intValue() in the initWindowManager where the values are read from storm conf.
  2. The corresponding constants defined in the Config classes like TOPOLOGY_BOLTS_WINDOW_LENGTH_DURATION_MS are validated for integer range since its annotated with @isInteger. This needs to be removed and appropriate annotation should be specified.

Its also good to add some unit tests to validate the changes.

satishd
satishd reviewed Jul 6, 2016
View reviewed changes
Comment threadstorm-core/src/jvm/org/apache/storm/topology/WindowedBoltExecutor.java
Count slidingIntervalCount, Duration slidingIntervalDuration) {

int topologyTimeout = getTopologyTimeoutMillis(stormConf);
long topologyTimeout = getTopologyTimeoutMillis(stormConf);

@satishdsatishdJul 6, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be int, toplogy-timeout-millis type is int.

@darionyaphet

darionyaphet commented Jul 14, 2016

Copy link
Copy Markdown
ContributorAuthor

@arunmahadevan why don't have @isLong ? millisecond using it to decorate is better

arunmahadevan
arunmahadevan reviewed Jul 15, 2016
View reviewed changes
Comment threadstorm-core/src/jvm/org/apache/storm/topology/WindowedBoltExecutor.java
private long getTopologyTimeoutMillis(Map stormConf) {
if (stormConf.get(Config.TOPOLOGY_ENABLE_MESSAGE_TIMEOUTS) != null) {
boolean timeOutsEnabled = (boolean) stormConf.get(Config.TOPOLOGY_ENABLE_MESSAGE_TIMEOUTS);
if (!timeOutsEnabled) {

@arunmahadevanarunmahadevanJul 15, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return Long.MAX_VALUE below.

@arunmahadevan

arunmahadevan commented Jul 15, 2016

Copy link
Copy Markdown
Contributor

Either you can use @isNumber or add a @isLong validator to decorate the millisecond values, its not already available.

d2r pushed a commit to d2r/storm that referenced this pull request Oct 16, 2018
Derek Dagit
Closes Pull Requests unchanged in 2018
fcda81c
We are closing stale Pull Requests to make the list more manageable.
Please re-open any Pull Request that has been closed in error.
Closesapache#608Closesapache#639Closesapache#640Closesapache#648Closesapache#662Closesapache#668Closesapache#692Closesapache#705Closesapache#724Closesapache#728Closesapache#730Closesapache#753Closesapache#803Closesapache#854Closesapache#922Closesapache#986Closesapache#992Closesapache#1019Closesapache#1040Closesapache#1041Closesapache#1043Closesapache#1046Closesapache#1051Closesapache#1078Closesapache#1146Closesapache#1164Closesapache#1165Closesapache#1178Closesapache#1213Closesapache#1225Closesapache#1258Closesapache#1259Closesapache#1268Closesapache#1272Closesapache#1277Closesapache#1278Closesapache#1288Closesapache#1296Closesapache#1328Closesapache#1342Closesapache#1353Closesapache#1370Closesapache#1376Closesapache#1391Closesapache#1395Closesapache#1399Closesapache#1406Closesapache#1410Closesapache#1422Closesapache#1427Closesapache#1443Closesapache#1462Closesapache#1468Closesapache#1483Closesapache#1506Closesapache#1509Closesapache#1515Closesapache#1520Closesapache#1521Closesapache#1525Closesapache#1527Closesapache#1544Closesapache#1550Closesapache#1566Closesapache#1569Closesapache#1570Closesapache#1575Closesapache#1580Closesapache#1584Closesapache#1591Closesapache#1600Closesapache#1611Closesapache#1613Closesapache#1639Closesapache#1703Closesapache#1711Closesapache#1719Closesapache#1737Closesapache#1760Closesapache#1767Closesapache#1768Closesapache#1785Closesapache#1799Closesapache#1822Closesapache#1824Closesapache#1844Closesapache#1874Closesapache#1918Closesapache#1928Closesapache#1937Closesapache#1942Closesapache#1951Closesapache#1957Closesapache#1963Closesapache#1964Closesapache#1965Closesapache#1967Closesapache#1968Closesapache#1971Closesapache#1985Closesapache#1986Closesapache#1998Closesapache#2031Closesapache#2032Closesapache#2071Closesapache#2076Closesapache#2108Closesapache#2119Closesapache#2128Closesapache#2142Closesapache#2174Closesapache#2206Closesapache#2297Closesapache#2322Closesapache#2332Closesapache#2341Closesapache#2377Closesapache#2414Closesapache#2469
d2r pushed a commit to d2r/storm that referenced this pull request Oct 16, 2018
Derek Dagit
Closes Pull Requests unchanged in 2018
fdb95d0
We are closing stale Pull Requests to make the list more manageable.
Please re-open any Pull Request that has been closed in error.
Closesapache#608Closesapache#639Closesapache#640Closesapache#648Closesapache#662Closesapache#668Closesapache#692Closesapache#705Closesapache#724Closesapache#728Closesapache#730Closesapache#753Closesapache#803Closesapache#854Closesapache#922Closesapache#986Closesapache#992Closesapache#1019Closesapache#1040Closesapache#1041Closesapache#1043Closesapache#1046Closesapache#1051Closesapache#1078Closesapache#1146Closesapache#1164Closesapache#1165Closesapache#1178Closesapache#1213Closesapache#1225Closesapache#1258Closesapache#1259Closesapache#1268Closesapache#1272Closesapache#1277Closesapache#1278Closesapache#1288Closesapache#1296Closesapache#1328Closesapache#1342Closesapache#1353Closesapache#1370Closesapache#1376Closesapache#1391Closesapache#1395Closesapache#1399Closesapache#1406Closesapache#1410Closesapache#1422Closesapache#1427Closesapache#1443Closesapache#1462Closesapache#1468Closesapache#1483Closesapache#1506Closesapache#1509Closesapache#1515Closesapache#1520Closesapache#1521Closesapache#1525Closesapache#1527Closesapache#1544Closesapache#1550Closesapache#1566Closesapache#1569Closesapache#1570Closesapache#1575Closesapache#1580Closesapache#1584Closesapache#1591Closesapache#1600Closesapache#1611Closesapache#1613Closesapache#1639Closesapache#1703Closesapache#1711Closesapache#1719Closesapache#1737Closesapache#1760Closesapache#1767Closesapache#1768Closesapache#1785Closesapache#1799Closesapache#1822Closesapache#1824Closesapache#1844Closesapache#1874Closesapache#1918Closesapache#1928Closesapache#1937Closesapache#1942Closesapache#1951Closesapache#1957Closesapache#1963Closesapache#1964Closesapache#1965Closesapache#1967Closesapache#1968Closesapache#1971Closesapache#1985Closesapache#1986Closesapache#1998Closesapache#2031Closesapache#2032Closesapache#2071Closesapache#2076Closesapache#2108Closesapache#2119Closesapache#2128Closesapache#2142Closesapache#2174Closesapache#2206Closesapache#2297Closesapache#2322Closesapache#2332Closesapache#2341Closesapache#2377Closesapache#2414Closesapache#2469
@d2rd2r mentioned this pull request Oct 16, 2018
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@darionyaphet@harshach@HeartSaVioR@arunmahadevan@satishd