10
\$\begingroup\$

I have a problem with some remote devices at a lot of my manufacturing sites. There are two major problems:

  1. the network is unreliable in terms of maintaining connections
  2. machines have duplicate IP addresses

Both of these are, naturally, catastrophic. To detect these conditions, I wrote a small program that loads a list of devices (and a description) and pings them all every 10 minutes, dumping the results to a couple log files.

I use icmp4j and OpenCSV as dependencies. Import statements are implied. The server running it will be a very beefy server: 24 core Xeon X5650s with 72GB of RAM. Obviously, this isn't the only thing on it, but suffice it to say we'd have to push pretty hard to cause a CPU or memory problem.

I'm interested in any real feedback. If there's a better way to do one of the tasks, if any problems jump out, anything that might bite me in the butt down the road. I'd also consider comments on style, obviously avoiding holy wars and respecting differences of opinion. I'm particularly interested in how I'm passing messages around, what can be done to clean that up because I fear it might get ugly as the program creeps bigger.

public class Main {
    // just a simple struct to attach a description to a hostname...
    static class Host {
        Host(String name, String desc) { this.name = name; this.desc = desc; }
        String name;
        String desc;
        public String toString() { return name + " " + desc; }
    }
    /**
     * 
     * @param args
     * @throws FileNotFoundException not expected because we should create any file we don't already have
     * @throws InterruptedException not expected because nothing should be interripting us
     */
    public static void main(String[] args) throws FileNotFoundException, InterruptedException {

        final List<Host> hosts = loadHosts();

        // keep track of IP addresses and what hosts they belong to
        // we find multiple hosts have the same IP address (WTF) so if we try to ping different hosts
        // and they hit the same IP, there's a problem!
        final Map<String, Host> ips = new HashMap<>();

        final SimpleDateFormat sdf = new SimpleDateFormat("yyyy-MM-dd-HH-mm-ss");

        // the date at which the current log started
        Calendar logDate = new GregorianCalendar();
        String dateStr = sdf.format(logDate.getTime());

        while (true) {
            Calendar currDate = new GregorianCalendar();
            // if we are on a new week, roll over the log
            if(currDate.get(Calendar.WEEK_OF_YEAR) != logDate.get(Calendar.WEEK_OF_YEAR)) {
                logDate = new GregorianCalendar();
                dateStr = sdf.format(logDate.getTime());
            }
            // we'll use this later and compare it to 'now'... 
            long then = System.currentTimeMillis();
            // create new or append to existing CSV log file - rollover happens here
            try (final CSVWriter log = new CSVWriter(new FileWriter(new File("log" + dateStr + ".csv"), true))) {

                for (final Host host : hosts) {
                    // ping the host 5 times - this is the critical "business logic" step
                    String[] data = ping(host, 5);
                    // convenient holder objects for the host
                    Host newHost = new Host(data[HOST_INDEX], data[DESC_INDEX]);
                    // fetch the old host associated with that IP
                    Host oldHost = ips.put(data[IP_INDEX], newHost);
                    // see if this doesn't pass the sniff test
                    if(oldHost != null &&  data[IP_INDEX] != NO_IP && !oldHost.equals(data[HOST_INDEX])) {
                        data[DUP_IP_INDEX] = "dupe";
                        // yeah I know we're logging every single dupe, but they don't happen often, so this is not going to kill us
                        try(FileWriter dupes = new FileWriter(new File("dupes.log"),true)){
                            dupes.write(new Date() + " " + oldHost + " and " + newHost + " share ip " + data[IP_INDEX]);
                            dupes.write("\n");
                            dupes.flush();
                            dupes.close();
                        }
                    }
                    // OpenCSV is super easy to work with, I picked it up in like ten minutes. Hurray!
                    log.writeNext(data);
                    // yes, we flush every time. In reality we don't have to do this, but we want to in case something errors during the next write
                    // this is going to write about 200 times every ten minutes, or an average of less than once a second.
                    // even if they all went through perfect it's going to take around 1 minute to do all 200 pings... the server is a beast, 
                    // this is nothing... if this becomes a performance or maintence problem we'll address it then
                    log.flush();

                }
            } catch (IOException e) {
                // aanndd i won't be doing much about this, because 1) i don't expect it to happen and 2) i can't really recover if it does
                e.printStackTrace();
            }
            long now = System.currentTimeMillis();
            long diff = now - then;
            // I love TimeUnit! It just makes this thoughtless!
            // we want to have a total of ten minutes between each run
            long durr = TimeUnit.MINUTES.toMillis(10); 
            // so we subtract how long we took on this trip and 
            long waitDurr = durr - diff;
            System.out.println("Finished run, waiting " + waitDurr + " milliseconds to run again");
            if (waitDurr > 100) {
                Thread.sleep(waitDurr);
            }

        }

    }

    // just some static values that make it SO much easier when you have to add a value in the middle
    static final String NO_IP = "no ip available";
    static final int HOST_INDEX = 0;
    static final int DESC_INDEX = 1;
    static final int IP_INDEX = 2;
    static final int DUP_IP_INDEX = 3;
    static final int TIMESTAMP_INDEX = 4;
    static final int DATE_INDEX = 5;
    static final int ERROR_INDEX = 6;
    static final int RESULTS_START = 7;
    static String[] ping(Host host, int attempts) {
        // Icmp4j - who would have thought? Turns out they can't do multi-threaded pings though, so I fear some of it
        // may be inaccurate - if I have to I can pump a ping command through the command line...
        IcmpPingRequest ping = IcmpPingUtil.createIcmpPingRequest();
        ping.setHost(host.name);
        ping.setTimeout(500);
        String[] results = new String[attempts+RESULTS_START];

        // prepopulate results matrix
        results[HOST_INDEX] = host.name;
        results[DESC_INDEX] = host.desc;
        results[IP_INDEX] = NO_IP;
        results[DUP_IP_INDEX] = "none";
        results[TIMESTAMP_INDEX] = String.valueOf(System.currentTimeMillis());
        results[DATE_INDEX] = new Date().toString();
        results[ERROR_INDEX] = ""; // yeah we're using string concat later... it's not pretty but it's low useage


        for (int i = RESULTS_START; i < results.length; i++) {
            try {
                IcmpPingResponse resp = IcmpPingUtil.executePingRequest(ping);
                results[IP_INDEX] = StringUtils.defaultString(resp.getHost(), results[IP_INDEX]);
                // this particular ping failed, mark as -1 and append its error message
                if(!resp.getSuccessFlag()) {
                    results[ERROR_INDEX] += resp.getErrorMessage() + "; ";
                    results[i] = "-1";
                } else {
                    results[i] = String.valueOf(resp.getDuration());
                }
            } catch (Exception e) {
                // the whole request failed, mark -1s and append its error message
                results[ERROR_INDEX] += e.getMessage() + "; ";
                Arrays.fill(results, RESULTS_START, results.length, "-1");
                return results;
            }
        }
        return results;
    }

    static List<Host> loadHosts() throws FileNotFoundException {
        List<Host> hosts = new ArrayList<Host>();
        try (Scanner sc = new Scanner(new File("hosts.txt"))) {
             while(sc.hasNextLine()) {
                 String hostline = sc.nextLine().trim();
                 // this allows us to 
                 // * skip blank lines
                 // * comments (starting with # or $ or something
                 // * NO_HOST, which I used to filter the data that was given to me to build the hosts file from
                 if(hostline.isEmpty() || !Character.isLetterOrDigit(hostline.charAt(0)) || hostline.contains("NO_HOST")) {
                     System.out.println("skipped host " + hostline);
                     continue;
                 }
                 // it is decreed that the first word of a valid line is its name, and everything after that is its description
                 String[] split = hostline.split("\\s+", 2);
                 String name = split[0];
                 String desc = split.length < 2 ? "" : split[1];
                 Host host = new Host(name,desc);
                 System.out.println("added host " + host);
                 hosts.add(host);
             }
        }
        System.out.println("number of hosts: " + hosts.size());
        return hosts;
    }

}
\$\endgroup\$
  • \$\begingroup\$ Are you on Java 8? \$\endgroup\$ – h.j.k. Jan 7 '16 at 1:21
  • 1
    \$\begingroup\$ My development environment is on Java 8, but my production environment isn't guaranteed to be, and I can't make changes without a big change management process, so let's go with no. If there was a super compelling reason to do so, I'd consider it. \$\endgroup\$ – corsiKa Jan 7 '16 at 2:51
1
\$\begingroup\$

Could replace for loop below:

for (final Host host : hosts) 
{
   // ping the host 5 times ...
   String[] data = ping(host, 5);
   ...
   ...
   log.flush();
}

Replace with Iterator<Host> or could use a Guava Function with existing List<Host> collection. transform method could apply Function, returning collection of desired String log messages.

Suggest usage of named groups within regular expression to replace split() code. Regular expression below can be maintained outside source code using properties collection, XML.

final String hostline = "desired-name whatever description details";

// it is decreed that the first word of a valid line is its name, and 
// everything after that is its description
final String[] split = hostline.split("\\s+", 2);
final String name = split[0];
final String desc = split.length < 2 ? "" : split[1];

Using regular expression, named groups instead:

final Pattern recordLayout = Pattern.compile(
        "(?<HostName>[^\\s]{1,})"
        + "[\\s]{1}"
        + "(?<HostDescription>[^\\n]{1,})");

final Matcher hostLineMatcher = recordLayout.matcher(hostline);

assertTrue( "Host line record invalid", hostLineMatcher.lookingAt() );

assertEquals( "Name unexpected",
            name,
            hostLineMatcher.group("HostName") );

assertEquals( "Description unexpected",
            desc,
            hostLineMatcher.group("HostDescription") );
| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ I like your second option of using a better defined pattern. I'm not sure I see the point of your first suggestion, though... the foreach loop for(final Host host : hosts) creates an Iterator<Host> object already. What do I gain by declaring the Iterator explicitly? \$\endgroup\$ – corsiKa Feb 3 '16 at 8:51
  • \$\begingroup\$ I usually employ explicit iterators, preferring the next method of the iterator, but you are right; there's no benefit I could argue. \$\endgroup\$ – kph0x1 Feb 3 '16 at 14:28

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.