Accumulo
  1. Accumulo
  2. ACCUMULO-2966

ZooReaderWriter.getInstance ignores changes in params

    Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Affects Version/s: 1.5.0, 1.5.1, 1.6.0
    • Fix Version/s: 1.8.0
    • Component/s: fate
    • Labels:

      Description

      the factory methods on ZooReaderWriter take parameters for servers, timeout, and authentication, but ignores if those parameters are different from the first invocation.

      
        public static synchronized ZooReaderWriter getInstance(String zookeepers, int timeInMillis, String scheme, byte[] auth) {
          if (instance == null)
            instance = new ZooReaderWriter(zookeepers, timeInMillis, scheme, auth);
          return instance;
        }
      
        /**
         * get an instance that retries when zookeeper connection errors occur
         * 
         * @return an instance that retries when Zookeeper connection errors occur.
         */
        public static synchronized IZooReaderWriter getRetryingInstance(String zookeepers, int timeInMillis, String scheme, byte[] auth) {
      
          if (retryingInstance == null) {
            IZooReaderWriter inst = getInstance(zookeepers, timeInMillis, scheme, auth);
            InvocationHandler ih = new RetryingInvocationHandler(inst);
            retryingInstance = (IZooReaderWriter) Proxy.newProxyInstance(ZooReaderWriter.class.getClassLoader(), new Class[] {IZooReaderWriter.class}, ih);
          }
      
          return retryingInstance;
        }
      

      It should either keep a cache keyed on the parameter values or it should throw an exception when they differ from the existing instance.

      Which one depends on wether the intent is to reuse objects or to have ZooReaderWriter be a proper singleton. I'm not sure from context, but I think the cache is the way to go.

        Issue Links

          Activity

          Hide
          Bill Havanki added a comment -

          ACCUMULO-2212 introduced a new ZooReaderWriterFactory class in 1.6.x and higher to replace this static factory method. The factory class does have the same issue, that of using the same instance for repeated calls, even with different parameters.

          The implementer here therefore has the option of backporting ZooReaderWriterFactory to 1.5.x as part of the work.

          Even though the factory class is available, it isn't widely used yet; those changes were too numerous for the original ticket. So, they could be included here, or this could be the impetus for a separate ticket to make those changes.

          Show
          Bill Havanki added a comment - ACCUMULO-2212 introduced a new ZooReaderWriterFactory class in 1.6.x and higher to replace this static factory method. The factory class does have the same issue, that of using the same instance for repeated calls, even with different parameters. The implementer here therefore has the option of backporting ZooReaderWriterFactory to 1.5.x as part of the work. Even though the factory class is available, it isn't widely used yet; those changes were too numerous for the original ticket. So, they could be included here, or this could be the impetus for a separate ticket to make those changes.
          Hide
          Sean Busbey added a comment -

          How about we go for:

          • backport factory to 1.5
          • fix behavior in the factory
          • change the static method in ZooReaderWriter to use the factory

          and leave the retool to switch to the factory in the wider code base as a follow on?

          Show
          Sean Busbey added a comment - How about we go for: backport factory to 1.5 fix behavior in the factory change the static method in ZooReaderWriter to use the factory and leave the retool to switch to the factory in the wider code base as a follow on?
          Hide
          Bill Havanki added a comment -

          Good plan.

          Show
          Bill Havanki added a comment - Good plan.
          Hide
          Christopher Tubbs added a comment -

          This issue was marked to be fixed for 1.5.3. However, no patch has been provided, and development on 1.5 is waning. Non-urgent issues are not likely to be addressed. Users are encouraged to upgrade to a newer version of Accumulo.

          It seems that this issue still has a plan agreed to in a previous comment, which involved backporting the factory class back to 1.5. Given the waning of 1.5 development, that part of the plan can probably be omitted.

          Show
          Christopher Tubbs added a comment - This issue was marked to be fixed for 1.5.3. However, no patch has been provided, and development on 1.5 is waning. Non-urgent issues are not likely to be addressed. Users are encouraged to upgrade to a newer version of Accumulo. It seems that this issue still has a plan agreed to in a previous comment, which involved backporting the factory class back to 1.5. Given the waning of 1.5 development, that part of the plan can probably be omitted.

            People

            • Assignee:
              Unassigned
              Reporter:
              Sean Busbey
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:

                Development