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

    https://github.com/apache/storm/pull/2881#discussion_r228300609
  
    --- Diff: storm-client/src/jvm/org/apache/storm/utils/Utils.java ---
    @@ -121,11 +121,68 @@
         private static String memoizedLocalHostnameString = null;
         public static final Pattern TOPOLOGY_KEY_PATTERN = 
Pattern.compile("^[\\w \\t\\._-]+$", Pattern.UNICODE_CHARACTER_CLASS);
     
    +    public static final String NUMA_MEMORY_IN_MB = "MemoryInMB";
    +    public static final String NUMA_CORES = "Cores";
    +    public static final String NUMAS_PORTS = "Ports";
    +    public static final String NUMA_ID = "Id";
    +    public static final String NUMAS_BASE = "Numas";
    +
         static {
             localConf = readStormConfig();
             serializationDelegate = getSerializationDelegate(localConf);
         }
     
    +    /**
    +     * Validate supervisor numa configuration.
    +     * @param stormConf stormConf
    +     * @return getValidatedNumaMap
    +     * @throws KeyNotFoundException
    +     */
    +    public static Map<String, Object> getValidatedNumaMap(Map<String, 
Object> stormConf) throws KeyNotFoundException {
    +        Map<String, Object> validatedNumaMap = new HashMap();
    +        Map<String, Object> supervisorNumaMap = (Map<String, Object>) 
stormConf.get(Config.SUPERVISOR_NUMA_META);
    +        if (supervisorNumaMap == null) return validatedNumaMap;
    +        if (!supervisorNumaMap.containsKey(NUMAS_BASE)) {
    +            throw new KeyNotFoundException("The numa configurations [" + 
NUMAS_BASE + "] is missing!");
    +        }
    +        List<Map> numaEntries = (List<Map>) 
supervisorNumaMap.get(NUMAS_BASE);
    +        if (numaEntries == null) return validatedNumaMap;
    +        for (Map numa : numaEntries) {
    +            for (String key : new String[]{NUMA_ID, NUMA_CORES, 
NUMA_MEMORY_IN_MB, NUMAS_PORTS}) {
    +                if (!numa.containsKey(key)) {
    +                    throw new KeyNotFoundException("The numa configuration 
key [" + key + "] is missing!");
    +                }
    +            }
    +            validatedNumaMap.put((String) numa.get(NUMA_ID), numa);
    +        }
    +        return validatedNumaMap;
    +    }
    +
    +    /**
    +     * getNumaIdForPort.
    +     * @param port port
    +     * @param stormConf stormConf
    +     * @return getNumaIdForPort
    +     * @throws KeyNotFoundException
    +     */
    +    public static String getNumaIdForPort(Number port, Map<String, Object> 
stormConf) {
    +        Map validatedNumaMap = null;
    +        try {
    +            validatedNumaMap = getValidatedNumaMap(stormConf);
    +        } catch (KeyNotFoundException e) {
    +            LOG.error("Exception while getting NUMA config", e);
    +            return null;
    --- End diff --
    
    I agree that we should throw an exception if it's misconfigured. If numa is 
configured, I think we want to make sure it's working properly or fail fast if 
misconfigured. Storm admins might not notice there is an "ERROR" message at the 
first place and wondering why storm is  not working as expected (after a 
relatively long time). If they don't want numa, they will not set numa configs. 


---

Reply via email to