STORM-3727 handle long values for SUPERVISOR_SLOTS_PORTS #3365

agresch · 2020-12-21T20:16:40Z

What is the purpose of the change

Apparently it is possible for supervisorConf.getOrDefault(DaemonConfig.SUPERVISOR_SLOTS_PORTS) to return a list of Long values, which caused a ClassCastException.

How was the change tested

Built storm-server jar.

Ethanlm · 2020-12-21T21:44:09Z

storm-server/src/main/java/org/apache/storm/daemon/supervisor/SupervisorUtils.java

                new ArrayList<>());
+        for (Number port : ports) {
+            slotsPorts.add(port.intValue());


This would may involve rounding or truncation which isn't really what we wanted.

A port by definition is no larger than 65535. So logically it will be Integers.

There is an Integer check here

storm/storm-server/src/main/java/org/apache/storm/DaemonConfig.java

Lines 742 to 743 in 6e37072

@IsListEntryCustom(entryValidatorClasses = { ConfigValidation.IntegerValidator.class, ConfigValidation.PositiveNumberValidator.class })

public static final String SUPERVISOR_SLOTS_PORTS = "supervisor.slots.ports";

. Not sure why it was not properly enforced.

This is similar to how the code used to be (see https://github.com/apache/storm/blame/69e69d40d5c700465b49f4c23312f974fb2ca1f6/storm-server/src/main/java/org/apache/storm/daemon/supervisor/ReadClusterState.java#L101-L103) before this change: https://github.com/apache/storm/pull/3267/files

How do we reproduce this?

I changed the port to be a value larger than Integer.MAX_VALUE, the supervisor fails to start

2020-12-21 22:50:47.009 o.a.s.v.ConfigValidation main [INFO] Will use [class org.apache.storm.DaemonConfig, class org.apache.storm.Config] for validation 2020-12-21 22:50:47.250 o.a.s.u.Utils main [ERROR] Received error in thread main.. terminating server... java.lang.Error: java.lang.IllegalArgumentException: Field SUPERVISOR_SLOTS_PORTS list entry must be an Integer within type range. at org.apache.storm.utils.Utils.handleUncaughtException(Utils.java:664) ~[storm-client-2.3.0.y.jar:2.3.0-SNAPSHOT] at org.apache.storm.utils.Utils.handleUncaughtException(Utils.java:668) ~[storm-client-2.3.0.y.jar:2.3.0-SNAPSHOT] at org.apache.storm.utils.Utils.lambda$createDefaultUncaughtExceptionHandler$2(Utils.java:1048) ~[storm-client-2.3.0.y.jar:2.3.0-SNAPSHOT] at java.lang.ThreadGroup.uncaughtException(ThreadGroup.java:1057) [?:1.8.0_262] at java.lang.ThreadGroup.uncaughtException(ThreadGroup.java:1052) [?:1.8.0_262] at java.lang.Thread.dispatchUncaughtException(Thread.java:1959) [?:1.8.0_262] Caused by: java.lang.IllegalArgumentException: Field SUPERVISOR_SLOTS_PORTS list entry must be an Integer within type range. at org.apache.storm.validation.ConfigValidation$IntegerValidator.validateInteger(ConfigValidation.java:415) ~[storm-client-2.3.0.y.jar:2.3.0-SNAPSHOT] at org.apache.storm.validation.ConfigValidation$IntegerValidator.validateField(ConfigValidation.java:401) ~[storm-client-2.3.0.y.jar:2.3.0-SNAPSHOT] at org.apache.storm.validation.ConfigValidation$ListEntryCustomValidator.validateField(ConfigValidation.java:588) ~[storm-client-2.3.0.y.jar:2.3.0-SNAPSHOT] at org.apache.storm.validation.ConfigValidation$ListEntryCustomValidator.validateField(ConfigValidation.java:602) ~[storm-client-2.3.0.y.jar:2.3.0-SNAPSHOT] at org.apache.storm.validation.ConfigValidation.validateField(ConfigValidation.java:167) ~[storm-client-2.3.0.y.jar:2.3.0-SNAPSHOT] at org.apache.storm.validation.ConfigValidation.validateFields(ConfigValidation.java:215) ~[storm-client-2.3.0.y.jar:2.3.0-SNAPSHOT] at org.apache.storm.validation.ConfigValidation.validateFields(ConfigValidation.java:189) ~[storm-client-2.3.0.y.jar:2.3.0-SNAPSHOT] at org.apache.storm.utils.ConfigUtils.readStormConfigImpl(ConfigUtils.java:440) ~[storm-client-2.3.0.y.jar:2.3.0-SNAPSHOT] at org.apache.storm.utils.ConfigUtils.readStormConfig(ConfigUtils.java:166) ~[storm-client-2.3.0.y.jar:2.3.0-SNAPSHOT] at org.apache.storm.daemon.supervisor.Supervisor.<init>(Supervisor.java:126) ~[storm-server-2.3.0.y.jar:2.3.0-SNAPSHOT] at org.apache.storm.daemon.supervisor.Supervisor.main(Supervisor.java:200) ~[storm-server-2.3.0.y.jar:2.3.0-SNAPSHOT]

Without this change:

./bin/storm supervisor -c supervisor.slots.ports="[6700]"
2020-12-28 14:06:38.860 o.a.s.d.s.Supervisor main [ERROR] Failed to start supervisor

java.lang.ClassCastException: java.lang.Long cannot be cast to java.lang.Integer
at org.apache.storm.daemon.supervisor.ReadClusterState.(ReadClusterState.java:101) ~[storm-server-2.3.0-SNAPSHOT.jar:2.3.0-SNAPSHOT]
at org.apache.storm.daemon.supervisor.Supervisor.launch(Supervisor.java:310) ~[storm-server-2.3.0-SNAPSHOT.jar:2.3.0-SNAPSHOT]
at org.apache.storm.daemon.supervisor.Supervisor.launchDaemon(Supervisor.java:340) [storm-server-2.3.0-SNAPSHOT.jar:2.3.0-SNAPSHOT]
at org.apache.storm.daemon.supervisor.Supervisor.main(Supervisor.java:201) [storm-server-2.3.0-SNAPSHOT.jar:2.3.0-SNAPSHOT]

Ethanlm · 2021-01-05T17:19:37Z

I think I know what's going on here now.

As @agresch mentioned, this issue can be reproduced by ./bin/storm supervisor -c supervisor.slots.ports="[6700]"

When supervisor starts, it reads defaults.yaml, storm.yaml and the command option, then validate the configs.

storm/storm-server/src/main/java/org/apache/storm/daemon/supervisor/Supervisor.java

Line 126 in 7bef73a

this(ConfigUtils.readStormConfig(), null, iSupervisor, metricsRegistry);

storm/storm-client/src/jvm/org/apache/storm/utils/ConfigUtils.java

Lines 438 to 442 in 7bef73a

    
           public Map<String, Object> readStormConfigImpl() { 
        
               Map<String, Object> conf = Utils.readStormConfig(); 
        
               ConfigValidation.validateFields(conf); 
        
               return conf; 
        
           }

However, the object type of supervisor.slots.ports interpreted by readCommandLineOpts, which uses json parser, is JSONArray and the element in the JSONArray is Long. The code is:

storm/storm-client/src/jvm/org/apache/storm/utils/Utils.java

Lines 287 to 298 in f451be2

    
           for (String config : configs) { 
        
               config = urlDecodeUtf8(config); 
        
               String[] options = config.split("=", 2); 
        
               if (options.length == 2) { 
        
                   Object val = options[1]; 
        
                   try { 
        
                       val = JSONValue.parseWithException(options[1]); 
        
                   } catch (ParseException ignored) { 
        
                       //fall back to string, which is already set 
        
                   } 
        
                   ret.put(options[0], val); 
        
               }

On the other hand, the object type of supervisor.slots.ports interpreted by findAndReadConfigFile, which uses Yaml, is ArrayList<List>. The code is

storm/storm-client/src/jvm/org/apache/storm/utils/Utils.java

Lines 179 to 182 in f451be2

    
           Yaml yaml = new Yaml(new SafeConstructor()); 
        
           @SuppressWarnings("unchecked") 
        
           Map<String, Object> ret = (Map<String, Object>) yaml.load(new InputStreamReader(in)); 
        
           if (null != ret) {

But the IntegerValidator doesn't enforce the object type to be Integer.

storm/storm-client/src/jvm/org/apache/storm/validation/ConfigValidation.java

Lines 404 to 415 in 7bef73a

    
           public void validateInteger(String name, Object o) { 
        
               if (o == null) { 
        
                   return; 
        
               } 
        
               final long i; 
        
               if (o instanceof Number 
        
                   && (i = ((Number) o).longValue()) == ((Number) o).doubleValue()) { 
        
                   if (i <= Integer.MAX_VALUE && i >= Integer.MIN_VALUE) { 
        
                       return; 
        
                   } 
        
               } 
        
               throw new IllegalArgumentException("Field " + name + " must be an Integer within type range.");

It will pass the check if the value has longValue() == doubleValue() and and the value is in the range of [Integer.MIN_VALUE, Integer.MAX_VALUE], even "1.0" meets the requirement.

Changing the IntegerValidator code to

public void validateField(String name, Object o) {
            SimpleTypeValidator.validateField(name, Integer.class, o);
        }

like

storm/storm-client/src/jvm/org/apache/storm/validation/ConfigValidation.java

Line 390 in 7bef73a

SimpleTypeValidator.validateField(name, Double.class, o);

will give us:

java.lang.Error: java.lang.IllegalArgumentException: Field SUPERVISOR_SLOTS_PORTS list entry must be of type class java.lang.Integer. Object: 6700 actual type: class java.lang.Long

when the command is ./bin/storm supervisor -c supervisor.slots.ports="[6700]"

With that being said, I am okay with reverting the code back to original to fix this issue STORM-3727.

Filed a separate jira at https://issues.apache.org/jira/browse/STORM-3734

STORM-3727 handle long values for SUPERVISOR_SLOTS_PORTS

2ca1bb4

RuiLi8080 approved these changes Dec 21, 2020

View reviewed changes

Ethanlm reviewed Dec 21, 2020

View reviewed changes

bipinprasad approved these changes Dec 22, 2020

View reviewed changes

bipinprasad approved these changes Dec 24, 2020

View reviewed changes

Ethanlm approved these changes Jan 5, 2021

View reviewed changes

Ethanlm merged commit ba7f969 into apache:master Jan 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

STORM-3727 handle long values for SUPERVISOR_SLOTS_PORTS #3365

STORM-3727 handle long values for SUPERVISOR_SLOTS_PORTS #3365

agresch commented Dec 21, 2020

Ethanlm Dec 21, 2020

agresch Dec 21, 2020

Ethanlm Dec 21, 2020

agresch Dec 28, 2020

Ethanlm Jan 5, 2021

Ethanlm commented Jan 5, 2021 •

edited

	@IsListEntryCustom(entryValidatorClasses = { ConfigValidation.IntegerValidator.class, ConfigValidation.PositiveNumberValidator.class })
	public static final String SUPERVISOR_SLOTS_PORTS = "supervisor.slots.ports";

STORM-3727 handle long values for SUPERVISOR_SLOTS_PORTS #3365

STORM-3727 handle long values for SUPERVISOR_SLOTS_PORTS #3365

Conversation

agresch commented Dec 21, 2020

What is the purpose of the change

How was the change tested

Ethanlm Dec 21, 2020

Choose a reason for hiding this comment

agresch Dec 21, 2020

Choose a reason for hiding this comment

Ethanlm Dec 21, 2020

Choose a reason for hiding this comment

agresch Dec 28, 2020

Choose a reason for hiding this comment

Ethanlm Jan 5, 2021

Choose a reason for hiding this comment

Ethanlm commented Jan 5, 2021 • edited

Ethanlm commented Jan 5, 2021 •

edited