From fbc58cd8be630a07b6718509ce419b83bb8971ff Mon Sep 17 00:00:00 2001 From: Keith Turner Date: Wed, 11 Feb 2026 02:14:25 +0000 Subject: [PATCH] fails on iterator configuration conflicts This is a follow on issue to #6040 that make all code warn when detecting iterator conflicts instead of logging at warn. --- .../clientImpl/NamespaceOperationsHelper.java | 3 +- .../clientImpl/TableOperationsHelper.java | 2 +- .../iteratorsImpl/IteratorConfigUtil.java | 30 +-- .../manager/thrift/ManagerClientService.java | 132 +++++++++- core/src/main/thrift/manager.thrift | 1 + .../server/conf/TableConfiguration.java | 4 +- .../accumulo/manager/FateServiceHandler.java | 20 +- .../tserver/tablet/ScanDataSource.java | 3 +- test/pom.xml | 8 - .../test/functional/IteratorConflictsIT.java | 239 ++++-------------- 10 files changed, 205 insertions(+), 237 deletions(-) diff --git a/core/src/main/java/org/apache/accumulo/core/clientImpl/NamespaceOperationsHelper.java b/core/src/main/java/org/apache/accumulo/core/clientImpl/NamespaceOperationsHelper.java index 07958dd0ed0..f27d28cfc1c 100644 --- a/core/src/main/java/org/apache/accumulo/core/clientImpl/NamespaceOperationsHelper.java +++ b/core/src/main/java/org/apache/accumulo/core/clientImpl/NamespaceOperationsHelper.java @@ -148,8 +148,7 @@ public void checkIteratorConflicts(String namespace, IteratorSetting setting, throw new NamespaceNotFoundException(null, namespace, null); } var props = this.getNamespaceProperties(namespace); - IteratorConfigUtil.checkIteratorConflicts("namespace:" + namespace, props, setting, scopes, - true); + IteratorConfigUtil.checkIteratorConflicts("namespace:" + namespace, props, setting, scopes); } @Override diff --git a/core/src/main/java/org/apache/accumulo/core/clientImpl/TableOperationsHelper.java b/core/src/main/java/org/apache/accumulo/core/clientImpl/TableOperationsHelper.java index 70a43b0e314..03be6dcb1cc 100644 --- a/core/src/main/java/org/apache/accumulo/core/clientImpl/TableOperationsHelper.java +++ b/core/src/main/java/org/apache/accumulo/core/clientImpl/TableOperationsHelper.java @@ -140,7 +140,7 @@ public static void checkIteratorConflicts(Map props, IteratorSett EnumSet scopes) throws AccumuloException { checkArgument(setting != null, "setting is null"); checkArgument(scopes != null, "scopes is null"); - IteratorConfigUtil.checkIteratorConflicts("", props, setting, scopes, true); + IteratorConfigUtil.checkIteratorConflicts("", props, setting, scopes); } @Override diff --git a/core/src/main/java/org/apache/accumulo/core/iteratorsImpl/IteratorConfigUtil.java b/core/src/main/java/org/apache/accumulo/core/iteratorsImpl/IteratorConfigUtil.java index 312fae8368a..94d08c4072b 100644 --- a/core/src/main/java/org/apache/accumulo/core/iteratorsImpl/IteratorConfigUtil.java +++ b/core/src/main/java/org/apache/accumulo/core/iteratorsImpl/IteratorConfigUtil.java @@ -288,8 +288,9 @@ public static void checkIteratorPriorityConflicts(String errorContext, // properties. if (iterProps.stream() .anyMatch(iterProp -> newProperties.containsKey(iterProp.getProperty()))) { - log.warn("For {}, newly set property introduced an iterator priority conflict : {}", - errorContext, iterProps); + throw new IllegalArgumentException(String.format( + "For %s, newly set property introduced an iterator priority conflict : %s", + errorContext, iterProps)); } } }); @@ -298,8 +299,7 @@ public static void checkIteratorPriorityConflicts(String errorContext, public static void checkIteratorConflicts(String logContext, IteratorSetting iterToCheck, EnumSet iterScopesToCheck, - Map> existingIters, boolean shouldThrow) - throws AccumuloException { + Map> existingIters) throws AccumuloException { // The reason for the 'shouldThrow' var is to prevent newly added 2.x checks from breaking // existing user code. Just log the problem and proceed. Major version > 2 will always throw for (var scope : iterScopesToCheck) { @@ -316,28 +316,20 @@ public static void checkIteratorConflicts(String logContext, IteratorSetting ite String msg = String.format("%s iterator name conflict at %s scope. %s conflicts with existing %s", logContext, scope, iterToCheck, existingIter); - if (shouldThrow) { - throw new AccumuloException(new IllegalArgumentException(msg)); - } else { - log.warn(msg + WARNING_MSG); - } + throw new AccumuloException(new IllegalArgumentException(msg)); } if (iterToCheck.getPriority() == existingIter.getPriority()) { String msg = String.format( "%s iterator priority conflict at %s scope. %s conflicts with existing %s", logContext, scope, iterToCheck, existingIter); - if (shouldThrow) { - throw new AccumuloException(new IllegalArgumentException(msg)); - } else { - log.warn(msg + WARNING_MSG); - } + throw new AccumuloException(new IllegalArgumentException(msg)); } } } } public static void checkIteratorConflicts(String logContext, Map props, - IteratorSetting iterToCheck, EnumSet iterScopesToCheck, boolean shouldThrow) + IteratorSetting iterToCheck, EnumSet iterScopesToCheck) throws AccumuloException { // parse the props map Map> iteratorSettings = new HashMap<>(); @@ -368,11 +360,7 @@ public static void checkIteratorConflicts(String logContext, Map String msg = String.format( "%s iterator name conflict at %s scope. %s conflicts with existing %s", logContext, iterProp.getScope(), iterToCheck, iterProp); - if (shouldThrow) { - throw new AccumuloException(new IllegalArgumentException(msg)); - } else { - log.warn(msg + WARNING_MSG); - } + throw new AccumuloException(new IllegalArgumentException(msg)); } } else { iterSetting.addOption(iterProp.getOptionKey(), iterProp.getOptionValue()); @@ -381,6 +369,6 @@ public static void checkIteratorConflicts(String logContext, Map } // check if the given iterator conflicts with any existing iterators - checkIteratorConflicts(logContext, iterToCheck, iterScopesToCheck, existingIters, shouldThrow); + checkIteratorConflicts(logContext, iterToCheck, iterScopesToCheck, existingIters); } } diff --git a/core/src/main/thrift-gen-java/org/apache/accumulo/core/manager/thrift/ManagerClientService.java b/core/src/main/thrift-gen-java/org/apache/accumulo/core/manager/thrift/ManagerClientService.java index dcdbd810588..9e845fbdcd3 100644 --- a/core/src/main/thrift-gen-java/org/apache/accumulo/core/manager/thrift/ManagerClientService.java +++ b/core/src/main/thrift-gen-java/org/apache/accumulo/core/manager/thrift/ManagerClientService.java @@ -41,7 +41,7 @@ public interface Iface { public void setNamespaceProperty(org.apache.accumulo.core.clientImpl.thrift.TInfo tinfo, org.apache.accumulo.core.securityImpl.thrift.TCredentials credentials, java.lang.String ns, java.lang.String property, java.lang.String value) throws org.apache.accumulo.core.clientImpl.thrift.ThriftSecurityException, org.apache.accumulo.core.clientImpl.thrift.ThriftTableOperationException, org.apache.accumulo.core.clientImpl.thrift.ThriftNotActiveServiceException, ThriftPropertyException, org.apache.thrift.TException; - public void modifyNamespaceProperties(org.apache.accumulo.core.clientImpl.thrift.TInfo tinfo, org.apache.accumulo.core.securityImpl.thrift.TCredentials credentials, java.lang.String ns, org.apache.accumulo.core.clientImpl.thrift.TVersionedProperties vProperties) throws org.apache.accumulo.core.clientImpl.thrift.ThriftSecurityException, org.apache.accumulo.core.clientImpl.thrift.ThriftTableOperationException, org.apache.accumulo.core.clientImpl.thrift.ThriftNotActiveServiceException, org.apache.accumulo.core.clientImpl.thrift.ThriftConcurrentModificationException, org.apache.thrift.TException; + public void modifyNamespaceProperties(org.apache.accumulo.core.clientImpl.thrift.TInfo tinfo, org.apache.accumulo.core.securityImpl.thrift.TCredentials credentials, java.lang.String ns, org.apache.accumulo.core.clientImpl.thrift.TVersionedProperties vProperties) throws org.apache.accumulo.core.clientImpl.thrift.ThriftSecurityException, org.apache.accumulo.core.clientImpl.thrift.ThriftTableOperationException, org.apache.accumulo.core.clientImpl.thrift.ThriftNotActiveServiceException, org.apache.accumulo.core.clientImpl.thrift.ThriftConcurrentModificationException, ThriftPropertyException, org.apache.thrift.TException; public void removeNamespaceProperty(org.apache.accumulo.core.clientImpl.thrift.TInfo tinfo, org.apache.accumulo.core.securityImpl.thrift.TCredentials credentials, java.lang.String ns, java.lang.String property) throws org.apache.accumulo.core.clientImpl.thrift.ThriftSecurityException, org.apache.accumulo.core.clientImpl.thrift.ThriftTableOperationException, org.apache.accumulo.core.clientImpl.thrift.ThriftNotActiveServiceException, org.apache.thrift.TException; @@ -387,7 +387,7 @@ public void recv_setNamespaceProperty() throws org.apache.accumulo.core.clientIm } @Override - public void modifyNamespaceProperties(org.apache.accumulo.core.clientImpl.thrift.TInfo tinfo, org.apache.accumulo.core.securityImpl.thrift.TCredentials credentials, java.lang.String ns, org.apache.accumulo.core.clientImpl.thrift.TVersionedProperties vProperties) throws org.apache.accumulo.core.clientImpl.thrift.ThriftSecurityException, org.apache.accumulo.core.clientImpl.thrift.ThriftTableOperationException, org.apache.accumulo.core.clientImpl.thrift.ThriftNotActiveServiceException, org.apache.accumulo.core.clientImpl.thrift.ThriftConcurrentModificationException, org.apache.thrift.TException + public void modifyNamespaceProperties(org.apache.accumulo.core.clientImpl.thrift.TInfo tinfo, org.apache.accumulo.core.securityImpl.thrift.TCredentials credentials, java.lang.String ns, org.apache.accumulo.core.clientImpl.thrift.TVersionedProperties vProperties) throws org.apache.accumulo.core.clientImpl.thrift.ThriftSecurityException, org.apache.accumulo.core.clientImpl.thrift.ThriftTableOperationException, org.apache.accumulo.core.clientImpl.thrift.ThriftNotActiveServiceException, org.apache.accumulo.core.clientImpl.thrift.ThriftConcurrentModificationException, ThriftPropertyException, org.apache.thrift.TException { send_modifyNamespaceProperties(tinfo, credentials, ns, vProperties); recv_modifyNamespaceProperties(); @@ -403,7 +403,7 @@ public void send_modifyNamespaceProperties(org.apache.accumulo.core.clientImpl.t sendBase("modifyNamespaceProperties", args); } - public void recv_modifyNamespaceProperties() throws org.apache.accumulo.core.clientImpl.thrift.ThriftSecurityException, org.apache.accumulo.core.clientImpl.thrift.ThriftTableOperationException, org.apache.accumulo.core.clientImpl.thrift.ThriftNotActiveServiceException, org.apache.accumulo.core.clientImpl.thrift.ThriftConcurrentModificationException, org.apache.thrift.TException + public void recv_modifyNamespaceProperties() throws org.apache.accumulo.core.clientImpl.thrift.ThriftSecurityException, org.apache.accumulo.core.clientImpl.thrift.ThriftTableOperationException, org.apache.accumulo.core.clientImpl.thrift.ThriftNotActiveServiceException, org.apache.accumulo.core.clientImpl.thrift.ThriftConcurrentModificationException, ThriftPropertyException, org.apache.thrift.TException { modifyNamespaceProperties_result result = new modifyNamespaceProperties_result(); receiveBase(result, "modifyNamespaceProperties"); @@ -419,6 +419,9 @@ public void recv_modifyNamespaceProperties() throws org.apache.accumulo.core.cli if (result.tcme != null) { throw result.tcme; } + if (result.tpe != null) { + throw result.tpe; + } return; } @@ -1400,7 +1403,7 @@ public void write_args(org.apache.thrift.protocol.TProtocol prot) throws org.apa } @Override - public Void getResult() throws org.apache.accumulo.core.clientImpl.thrift.ThriftSecurityException, org.apache.accumulo.core.clientImpl.thrift.ThriftTableOperationException, org.apache.accumulo.core.clientImpl.thrift.ThriftNotActiveServiceException, org.apache.accumulo.core.clientImpl.thrift.ThriftConcurrentModificationException, org.apache.thrift.TException { + public Void getResult() throws org.apache.accumulo.core.clientImpl.thrift.ThriftSecurityException, org.apache.accumulo.core.clientImpl.thrift.ThriftTableOperationException, org.apache.accumulo.core.clientImpl.thrift.ThriftNotActiveServiceException, org.apache.accumulo.core.clientImpl.thrift.ThriftConcurrentModificationException, ThriftPropertyException, org.apache.thrift.TException { if (getState() != org.apache.thrift.async.TAsyncMethodCall.State.RESPONSE_READ) { throw new java.lang.IllegalStateException("Method call not finished!"); } @@ -2610,6 +2613,8 @@ public modifyNamespaceProperties_result getResult(I iface, modifyNamespaceProper result.tnase = tnase; } catch (org.apache.accumulo.core.clientImpl.thrift.ThriftConcurrentModificationException tcme) { result.tcme = tcme; + } catch (ThriftPropertyException tpe) { + result.tpe = tpe; } return result; } @@ -3919,6 +3924,10 @@ public void onError(java.lang.Exception e) { result.tcme = (org.apache.accumulo.core.clientImpl.thrift.ThriftConcurrentModificationException) e; result.setTcmeIsSet(true); msg = result; + } else if (e instanceof ThriftPropertyException) { + result.tpe = (ThriftPropertyException) e; + result.setTpeIsSet(true); + msg = result; } else if (e instanceof org.apache.thrift.transport.TTransportException) { _LOGGER.error("TTransportException inside handler", e); fb.close(); @@ -14989,6 +14998,7 @@ public static class modifyNamespaceProperties_result implements org.apache.thrif private static final org.apache.thrift.protocol.TField TOPE_FIELD_DESC = new org.apache.thrift.protocol.TField("tope", org.apache.thrift.protocol.TType.STRUCT, (short)2); private static final org.apache.thrift.protocol.TField TNASE_FIELD_DESC = new org.apache.thrift.protocol.TField("tnase", org.apache.thrift.protocol.TType.STRUCT, (short)3); private static final org.apache.thrift.protocol.TField TCME_FIELD_DESC = new org.apache.thrift.protocol.TField("tcme", org.apache.thrift.protocol.TType.STRUCT, (short)4); + private static final org.apache.thrift.protocol.TField TPE_FIELD_DESC = new org.apache.thrift.protocol.TField("tpe", org.apache.thrift.protocol.TType.STRUCT, (short)5); private static final org.apache.thrift.scheme.SchemeFactory STANDARD_SCHEME_FACTORY = new modifyNamespaceProperties_resultStandardSchemeFactory(); private static final org.apache.thrift.scheme.SchemeFactory TUPLE_SCHEME_FACTORY = new modifyNamespaceProperties_resultTupleSchemeFactory(); @@ -14997,13 +15007,15 @@ public static class modifyNamespaceProperties_result implements org.apache.thrif public @org.apache.thrift.annotation.Nullable org.apache.accumulo.core.clientImpl.thrift.ThriftTableOperationException tope; // required public @org.apache.thrift.annotation.Nullable org.apache.accumulo.core.clientImpl.thrift.ThriftNotActiveServiceException tnase; // required public @org.apache.thrift.annotation.Nullable org.apache.accumulo.core.clientImpl.thrift.ThriftConcurrentModificationException tcme; // required + public @org.apache.thrift.annotation.Nullable ThriftPropertyException tpe; // required /** The set of fields this struct contains, along with convenience methods for finding and manipulating them. */ public enum _Fields implements org.apache.thrift.TFieldIdEnum { SEC((short)1, "sec"), TOPE((short)2, "tope"), TNASE((short)3, "tnase"), - TCME((short)4, "tcme"); + TCME((short)4, "tcme"), + TPE((short)5, "tpe"); private static final java.util.Map byName = new java.util.HashMap(); @@ -15027,6 +15039,8 @@ public static _Fields findByThriftId(int fieldId) { return TNASE; case 4: // TCME return TCME; + case 5: // TPE + return TPE; default: return null; } @@ -15081,6 +15095,8 @@ public java.lang.String getFieldName() { new org.apache.thrift.meta_data.StructMetaData(org.apache.thrift.protocol.TType.STRUCT, org.apache.accumulo.core.clientImpl.thrift.ThriftNotActiveServiceException.class))); tmpMap.put(_Fields.TCME, new org.apache.thrift.meta_data.FieldMetaData("tcme", org.apache.thrift.TFieldRequirementType.DEFAULT, new org.apache.thrift.meta_data.StructMetaData(org.apache.thrift.protocol.TType.STRUCT, org.apache.accumulo.core.clientImpl.thrift.ThriftConcurrentModificationException.class))); + tmpMap.put(_Fields.TPE, new org.apache.thrift.meta_data.FieldMetaData("tpe", org.apache.thrift.TFieldRequirementType.DEFAULT, + new org.apache.thrift.meta_data.StructMetaData(org.apache.thrift.protocol.TType.STRUCT, ThriftPropertyException.class))); metaDataMap = java.util.Collections.unmodifiableMap(tmpMap); org.apache.thrift.meta_data.FieldMetaData.addStructMetaDataMap(modifyNamespaceProperties_result.class, metaDataMap); } @@ -15092,13 +15108,15 @@ public modifyNamespaceProperties_result( org.apache.accumulo.core.clientImpl.thrift.ThriftSecurityException sec, org.apache.accumulo.core.clientImpl.thrift.ThriftTableOperationException tope, org.apache.accumulo.core.clientImpl.thrift.ThriftNotActiveServiceException tnase, - org.apache.accumulo.core.clientImpl.thrift.ThriftConcurrentModificationException tcme) + org.apache.accumulo.core.clientImpl.thrift.ThriftConcurrentModificationException tcme, + ThriftPropertyException tpe) { this(); this.sec = sec; this.tope = tope; this.tnase = tnase; this.tcme = tcme; + this.tpe = tpe; } /** @@ -15117,6 +15135,9 @@ public modifyNamespaceProperties_result(modifyNamespaceProperties_result other) if (other.isSetTcme()) { this.tcme = new org.apache.accumulo.core.clientImpl.thrift.ThriftConcurrentModificationException(other.tcme); } + if (other.isSetTpe()) { + this.tpe = new ThriftPropertyException(other.tpe); + } } @Override @@ -15130,6 +15151,7 @@ public void clear() { this.tope = null; this.tnase = null; this.tcme = null; + this.tpe = null; } @org.apache.thrift.annotation.Nullable @@ -15232,6 +15254,31 @@ public void setTcmeIsSet(boolean value) { } } + @org.apache.thrift.annotation.Nullable + public ThriftPropertyException getTpe() { + return this.tpe; + } + + public modifyNamespaceProperties_result setTpe(@org.apache.thrift.annotation.Nullable ThriftPropertyException tpe) { + this.tpe = tpe; + return this; + } + + public void unsetTpe() { + this.tpe = null; + } + + /** Returns true if field tpe is set (has been assigned a value) and false otherwise */ + public boolean isSetTpe() { + return this.tpe != null; + } + + public void setTpeIsSet(boolean value) { + if (!value) { + this.tpe = null; + } + } + @Override public void setFieldValue(_Fields field, @org.apache.thrift.annotation.Nullable java.lang.Object value) { switch (field) { @@ -15267,6 +15314,14 @@ public void setFieldValue(_Fields field, @org.apache.thrift.annotation.Nullable } break; + case TPE: + if (value == null) { + unsetTpe(); + } else { + setTpe((ThriftPropertyException)value); + } + break; + } } @@ -15286,6 +15341,9 @@ public java.lang.Object getFieldValue(_Fields field) { case TCME: return getTcme(); + case TPE: + return getTpe(); + } throw new java.lang.IllegalStateException(); } @@ -15306,6 +15364,8 @@ public boolean isSet(_Fields field) { return isSetTnase(); case TCME: return isSetTcme(); + case TPE: + return isSetTpe(); } throw new java.lang.IllegalStateException(); } @@ -15359,6 +15419,15 @@ public boolean equals(modifyNamespaceProperties_result that) { return false; } + boolean this_present_tpe = true && this.isSetTpe(); + boolean that_present_tpe = true && that.isSetTpe(); + if (this_present_tpe || that_present_tpe) { + if (!(this_present_tpe && that_present_tpe)) + return false; + if (!this.tpe.equals(that.tpe)) + return false; + } + return true; } @@ -15382,6 +15451,10 @@ public int hashCode() { if (isSetTcme()) hashCode = hashCode * 8191 + tcme.hashCode(); + hashCode = hashCode * 8191 + ((isSetTpe()) ? 131071 : 524287); + if (isSetTpe()) + hashCode = hashCode * 8191 + tpe.hashCode(); + return hashCode; } @@ -15433,6 +15506,16 @@ public int compareTo(modifyNamespaceProperties_result other) { return lastComparison; } } + lastComparison = java.lang.Boolean.compare(isSetTpe(), other.isSetTpe()); + if (lastComparison != 0) { + return lastComparison; + } + if (isSetTpe()) { + lastComparison = org.apache.thrift.TBaseHelper.compareTo(this.tpe, other.tpe); + if (lastComparison != 0) { + return lastComparison; + } + } return 0; } @@ -15487,6 +15570,14 @@ public java.lang.String toString() { sb.append(this.tcme); } first = false; + if (!first) sb.append(", "); + sb.append("tpe:"); + if (this.tpe == null) { + sb.append("null"); + } else { + sb.append(this.tpe); + } + first = false; sb.append(")"); return sb.toString(); } @@ -15568,6 +15659,15 @@ public void read(org.apache.thrift.protocol.TProtocol iprot, modifyNamespaceProp org.apache.thrift.protocol.TProtocolUtil.skip(iprot, schemeField.type); } break; + case 5: // TPE + if (schemeField.type == org.apache.thrift.protocol.TType.STRUCT) { + struct.tpe = new ThriftPropertyException(); + struct.tpe.read(iprot); + struct.setTpeIsSet(true); + } else { + org.apache.thrift.protocol.TProtocolUtil.skip(iprot, schemeField.type); + } + break; default: org.apache.thrift.protocol.TProtocolUtil.skip(iprot, schemeField.type); } @@ -15604,6 +15704,11 @@ public void write(org.apache.thrift.protocol.TProtocol oprot, modifyNamespacePro struct.tcme.write(oprot); oprot.writeFieldEnd(); } + if (struct.tpe != null) { + oprot.writeFieldBegin(TPE_FIELD_DESC); + struct.tpe.write(oprot); + oprot.writeFieldEnd(); + } oprot.writeFieldStop(); oprot.writeStructEnd(); } @@ -15635,7 +15740,10 @@ public void write(org.apache.thrift.protocol.TProtocol prot, modifyNamespaceProp if (struct.isSetTcme()) { optionals.set(3); } - oprot.writeBitSet(optionals, 4); + if (struct.isSetTpe()) { + optionals.set(4); + } + oprot.writeBitSet(optionals, 5); if (struct.isSetSec()) { struct.sec.write(oprot); } @@ -15648,12 +15756,15 @@ public void write(org.apache.thrift.protocol.TProtocol prot, modifyNamespaceProp if (struct.isSetTcme()) { struct.tcme.write(oprot); } + if (struct.isSetTpe()) { + struct.tpe.write(oprot); + } } @Override public void read(org.apache.thrift.protocol.TProtocol prot, modifyNamespaceProperties_result struct) throws org.apache.thrift.TException { org.apache.thrift.protocol.TTupleProtocol iprot = (org.apache.thrift.protocol.TTupleProtocol) prot; - java.util.BitSet incoming = iprot.readBitSet(4); + java.util.BitSet incoming = iprot.readBitSet(5); if (incoming.get(0)) { struct.sec = new org.apache.accumulo.core.clientImpl.thrift.ThriftSecurityException(); struct.sec.read(iprot); @@ -15674,6 +15785,11 @@ public void read(org.apache.thrift.protocol.TProtocol prot, modifyNamespacePrope struct.tcme.read(iprot); struct.setTcmeIsSet(true); } + if (incoming.get(4)) { + struct.tpe = new ThriftPropertyException(); + struct.tpe.read(iprot); + struct.setTpeIsSet(true); + } } } diff --git a/core/src/main/thrift/manager.thrift b/core/src/main/thrift/manager.thrift index 58ba9cdae0b..436d365e979 100644 --- a/core/src/main/thrift/manager.thrift +++ b/core/src/main/thrift/manager.thrift @@ -329,6 +329,7 @@ service ManagerClientService { 2:client.ThriftTableOperationException tope 3:client.ThriftNotActiveServiceException tnase 4:client.ThriftConcurrentModificationException tcme + 5:ThriftPropertyException tpe ) void removeNamespaceProperty( diff --git a/server/base/src/main/java/org/apache/accumulo/server/conf/TableConfiguration.java b/server/base/src/main/java/org/apache/accumulo/server/conf/TableConfiguration.java index 7f68d4ddf2e..5471663417b 100644 --- a/server/base/src/main/java/org/apache/accumulo/server/conf/TableConfiguration.java +++ b/server/base/src/main/java/org/apache/accumulo/server/conf/TableConfiguration.java @@ -80,8 +80,8 @@ public TableConfiguration(ServerContext context, TableId tableId, NamespaceConfi // This code assumes the list of iterators is sorted on priority Preconditions.checkState(last.getPriority() <= curr.getPriority()); if (last.getPriority() == curr.getPriority()) { - // duplicate priority - log.warn("iterator priority conflict seen for tableId:{} {} {}", tableId, last, curr); + throw new IllegalStateException(String.format( + "iterator priority conflict seen for tableId:%s %s %s", tableId, last, curr)); } } } diff --git a/server/manager/src/main/java/org/apache/accumulo/manager/FateServiceHandler.java b/server/manager/src/main/java/org/apache/accumulo/manager/FateServiceHandler.java index 1a9d0b43935..6c31e9174b7 100644 --- a/server/manager/src/main/java/org/apache/accumulo/manager/FateServiceHandler.java +++ b/server/manager/src/main/java/org/apache/accumulo/manager/FateServiceHandler.java @@ -238,8 +238,13 @@ public void executeFateOperation(TInfo tinfo, TCredentials c, TFateId opid, TFat var namespaceIterProps = manager.getContext().getNamespaceConfiguration(namespaceId) .getAllPropertiesWithPrefix(Property.TABLE_ITERATOR_PREFIX); - IteratorConfigUtil.checkIteratorPriorityConflicts("create table:" + tableName, options, - namespaceIterProps); + try { + IteratorConfigUtil.checkIteratorPriorityConflicts("create table:" + tableName, options, + namespaceIterProps); + } catch (IllegalArgumentException iae) { + throw new ThriftTableOperationException(null, tableName, tableOp, + TableOperationExceptionType.OTHER, iae.getMessage()); + } for (Map.Entry entry : options.entrySet()) { validateTableProperty(entry.getKey(), entry.getValue(), tableName, tableOp); } @@ -351,9 +356,14 @@ public void executeFateOperation(TInfo tinfo, TCredentials c, TFateId opid, TFat srcTableProps.keySet().removeAll(propertiesToExclude); var namespaceProps = manager.getContext().getNamespaceConfiguration(namespaceId) .getAllPropertiesWithPrefix(Property.TABLE_ITERATOR_PREFIX); - IteratorConfigUtil.checkIteratorPriorityConflicts( - "clone table source tableId:" + srcTableId + " tablename:" + tableName, srcTableProps, - namespaceProps); + try { + IteratorConfigUtil.checkIteratorPriorityConflicts( + "clone table source tableId:" + srcTableId + " tablename:" + tableName, srcTableProps, + namespaceProps); + } catch (IllegalArgumentException iae) { + throw new ThriftTableOperationException(null, tableName, tableOp, + TableOperationExceptionType.OTHER, iae.getMessage()); + } goalMessage += "Clone table " + srcTableId + " to " + tableName; if (keepOffline) { diff --git a/server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/ScanDataSource.java b/server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/ScanDataSource.java index 1bc88c4b291..a4e15686b61 100644 --- a/server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/ScanDataSource.java +++ b/server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/ScanDataSource.java @@ -265,8 +265,7 @@ private SortedKeyValueIterator createIterator() IteratorConfigUtil.checkIteratorConflicts(tablet.getExtent().toString(), getIteratorSetting(scanParamIterInfo, scanParams.getSsio().get(scanParamIterInfo.getIterName())), - EnumSet.of(IteratorScope.scan), Map.of(IteratorScope.scan, picIteratorSettings), - false); + EnumSet.of(IteratorScope.scan), Map.of(IteratorScope.scan, picIteratorSettings)); } catch (AccumuloException e) { throw new IllegalArgumentException(e); } diff --git a/test/pom.xml b/test/pom.xml index 8ba183cc63b..f986690c225 100644 --- a/test/pom.xml +++ b/test/pom.xml @@ -171,14 +171,6 @@ - - org.apache.logging.log4j - log4j-api - - - org.apache.logging.log4j - log4j-core - org.apache.thrift libthrift diff --git a/test/src/main/java/org/apache/accumulo/test/functional/IteratorConflictsIT.java b/test/src/main/java/org/apache/accumulo/test/functional/IteratorConflictsIT.java index 483af3bb038..108332b4925 100644 --- a/test/src/main/java/org/apache/accumulo/test/functional/IteratorConflictsIT.java +++ b/test/src/main/java/org/apache/accumulo/test/functional/IteratorConflictsIT.java @@ -18,23 +18,15 @@ */ package org.apache.accumulo.test.functional; -import static java.nio.charset.StandardCharsets.UTF_8; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; -import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; -import java.io.BufferedReader; -import java.io.InputStreamReader; -import java.time.LocalDateTime; -import java.time.format.DateTimeFormatter; -import java.util.ArrayList; import java.util.EnumSet; import java.util.List; import java.util.Map; import java.util.Set; -import java.util.concurrent.CopyOnWriteArrayList; import java.util.function.Consumer; import org.apache.accumulo.core.client.Accumulo; @@ -51,14 +43,6 @@ import org.apache.accumulo.core.iteratorsImpl.IteratorConfigUtil; import org.apache.accumulo.harness.SharedMiniClusterBase; import org.apache.accumulo.test.util.Wait; -import org.apache.hadoop.fs.Path; -import org.apache.logging.log4j.Level; -import org.apache.logging.log4j.LogManager; -import org.apache.logging.log4j.core.LogEvent; -import org.apache.logging.log4j.core.LoggerContext; -import org.apache.logging.log4j.core.appender.AbstractAppender; -import org.apache.logging.log4j.core.config.Configuration; -import org.apache.logging.log4j.core.layout.PatternLayout; import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; @@ -140,47 +124,16 @@ public class IteratorConflictsIT extends SharedMiniClusterBase { private static final String defaultIterOptVal = defaultTableIter.getOptions().entrySet().iterator().next().getValue(); - private static final LoggerContext loggerContext = (LoggerContext) LogManager.getContext(false); - private static final Configuration loggerConfig = loggerContext.getConfiguration(); - private static final TestAppender appender = new TestAppender(); - private static final String datePattern = getDatePattern(); - private static final DateTimeFormatter dateTimeFormatter = - DateTimeFormatter.ofPattern(datePattern); - - public static class TestAppender extends AbstractAppender { - // CopyOnWriteArrayList for thread safety, even while iterating - private final List events = new CopyOnWriteArrayList<>(); - - public TestAppender() { - super("TestAppender", null, PatternLayout.createDefaultLayout(), false, null); - } - - @Override - public void append(LogEvent event) { - events.add(event.toImmutable()); - } - - public List events() { - return events; - } - } - @BeforeAll public static void startup() throws Exception { SharedMiniClusterBase.startMiniCluster(); client = Accumulo.newClient().from(getClientProps()).build(); tops = client.tableOperations(); nops = client.namespaceOperations(); - appender.start(); - loggerConfig.getRootLogger().addAppender(appender, Level.WARN, null); - loggerContext.updateLoggers(); } @AfterAll public static void shutdown() throws Exception { - loggerConfig.getRootLogger().removeAppender(appender.getName()); - appender.stop(); - loggerContext.updateLoggers(); client.close(); SharedMiniClusterBase.stopMiniCluster(); } @@ -220,11 +173,11 @@ public void testTableIterConflict() throws Throwable { // testing TableOperations.setProperty testTableIterPrioConflict(table2, AccumuloException.class, - () -> tops.setProperty(table2, iter1PrioConflictKey, iter1PrioConflictVal), false); + () -> tops.setProperty(table2, iter1PrioConflictKey, iter1PrioConflictVal), true); // testing TableOperations.modifyProperties testTableIterPrioConflict(table3, AccumuloException.class, () -> tops.modifyProperties(table3, - props -> props.put(iter1PrioConflictKey, iter1PrioConflictVal)), false); + props -> props.put(iter1PrioConflictKey, iter1PrioConflictVal)), true); // NewTableConfiguration.attachIterator is not applicable for this test // Attaching the iterator to the table requires the table to exist, but testing @@ -237,7 +190,7 @@ public void testTableIterConflict() throws Throwable { () -> tops.attachIterator(table4, iter1NameConflict), true); // testing NamespaceOperations.attachIterator - testTableIterPrioConflict(table5, AccumuloException.class, () -> { + testTableIterPrioConflict(table5, IllegalStateException.class, () -> { nops.attachIterator(ns5, iter1PrioConflict); try (var scanner = client.createScanner(table5)) { assertFalse(scanner.iterator().hasNext()); @@ -245,7 +198,7 @@ public void testTableIterConflict() throws Throwable { }, false); // testing NamespaceOperations.setProperty - testTableIterPrioConflict(table6, AccumuloException.class, () -> { + testTableIterPrioConflict(table6, IllegalStateException.class, () -> { nops.setProperty(ns6, iter1PrioConflictKey, iter1PrioConflictVal); try (var scanner = client.createScanner(table6)) { assertFalse(scanner.iterator().hasNext()); @@ -253,7 +206,7 @@ public void testTableIterConflict() throws Throwable { }, false); // testing NamespaceOperations.modifyProperties - testTableIterPrioConflict(table7, AccumuloException.class, () -> { + testTableIterPrioConflict(table7, IllegalStateException.class, () -> { nops.modifyProperties(ns7, props -> props.put(iter1PrioConflictKey, iter1PrioConflictVal)); try (var scanner = client.createScanner(table7)) { assertFalse(scanner.iterator().hasNext()); @@ -266,36 +219,32 @@ public void testTableIterConflict() throws Throwable { () -> tops.clone(table8, table9, CloneConfiguration.builder() .setPropertiesToSet(Map.of(iter1PrioConflictKey, iter1PrioConflictVal)).build()), - false); + true); } private void testTableIterNameConflict(String table, - Class exceptionClass, Executable iterNameConflictExec, boolean shouldThrow) + Class exceptionClass, Executable iterNameConflictExec, boolean checkMessage) throws Throwable { - if (shouldThrow) { - var e = assertThrows(exceptionClass, iterNameConflictExec); + var e = assertThrows(exceptionClass, iterNameConflictExec); + if (checkMessage) { assertTrue(e.toString().contains("iterator name conflict")); - assertEquals(Set.of(iter1.getName(), "vers"), tops.listIterators(table).keySet()); - for (var scope : IteratorScope.values()) { - assertEquals(iter1, tops.getIteratorSetting(table, iter1.getName(), scope)); - } - } else { - assertTrue(logsContain(List.of("iterator name conflict"), iterNameConflictExec)); + } + assertEquals(Set.of(iter1.getName(), "vers"), tops.listIterators(table).keySet()); + for (var scope : IteratorScope.values()) { + assertEquals(iter1, tops.getIteratorSetting(table, iter1.getName(), scope)); } } private void testTableIterPrioConflict(String table, - Class exceptionClass, Executable iterPrioConflictExec, boolean shouldThrow) + Class exceptionClass, Executable iterPrioConflictExec, boolean checkMessage) throws Throwable { - if (shouldThrow) { - var e = assertThrows(exceptionClass, iterPrioConflictExec); + var e = assertThrows(exceptionClass, iterPrioConflictExec); + if (checkMessage) { assertTrue(e.toString().contains("iterator priority conflict")); assertEquals(Set.of(iter1.getName(), "vers"), tops.listIterators(table).keySet()); for (var scope : IteratorScope.values()) { assertEquals(iter1, tops.getIteratorSetting(table, iter1.getName(), scope)); } - } else { - assertTrue(logsContain(List.of("iterator priority conflict"), iterPrioConflictExec)); } } @@ -325,9 +274,7 @@ public void testNamespaceIterConflict() throws Throwable { String table2 = ns2 + "." + names[3]; tops.create(table2); testNamespaceIterPrioConflict(ns2, AccumuloException.class, - () -> tops.setProperty(table2, iter1PrioConflictKey, iter1PrioConflictVal), false); - - // TODO its ok to set a name that "conflicts" if it overrides in the merged view + () -> tops.setProperty(table2, iter1PrioConflictKey, iter1PrioConflictVal), true); // testing TableOperations.modifyProperties String ns3 = names[4]; @@ -335,7 +282,7 @@ public void testNamespaceIterConflict() throws Throwable { String table3 = ns3 + "." + names[5]; tops.create(table3); testNamespaceIterPrioConflict(ns3, AccumuloException.class, () -> tops.modifyProperties(table3, - props -> props.put(iter1PrioConflictKey, iter1PrioConflictVal)), false); + props -> props.put(iter1PrioConflictKey, iter1PrioConflictVal)), true); // testing NewTableConfiguration.attachIterator String ns4 = names[6]; @@ -343,7 +290,7 @@ public void testNamespaceIterConflict() throws Throwable { String table4 = ns4 + "." + names[7]; testNamespaceIterPrioConflict(ns4, AccumuloException.class, () -> tops.create(table4, new NewTableConfiguration().attachIterator(iter1PrioConflict)), - false); + true); // testing TableOperations.attachIterator String ns5 = names[9]; @@ -367,7 +314,7 @@ public void testNamespaceIterConflict() throws Throwable { String ns7 = names[12]; nops.create(ns7); testNamespaceIterPrioConflict(ns7, AccumuloException.class, - () -> nops.setProperty(ns7, iter1PrioConflictKey, iter1PrioConflictVal), false); + () -> nops.setProperty(ns7, iter1PrioConflictKey, iter1PrioConflictVal), true); // testing NamespaceOperations.modifyProperties String ns8 = names[13]; @@ -376,7 +323,7 @@ public void testNamespaceIterConflict() throws Throwable { () -> nops.modifyProperties(ns8, props -> props.put(iter1PrioConflictKey, iter1PrioConflictVal)), - false); + true); // testing CloneConfiguration.Builder.setPropertiesToSet // testing same src and dst namespace: should conflict @@ -390,7 +337,7 @@ public void testNamespaceIterConflict() throws Throwable { () -> tops.clone(src1, dst1, CloneConfiguration.builder() .setPropertiesToSet(Map.of(iter1PrioConflictKey, iter1PrioConflictVal)).build()), - false); + true); // testing attached to src namespace, different dst namespace: should not conflict String srcNamespace2 = names[18]; nops.create(srcNamespace2); @@ -419,42 +366,37 @@ public void testNamespaceIterConflict() throws Throwable { () -> tops.clone(src3, dst5, CloneConfiguration.builder() .setPropertiesToSet(Map.of(iter1PrioConflictKey, iter1PrioConflictVal)).build()), - false); + true); } private void testNamespaceIterPrioConflict(String ns, - Class exceptionClass, Executable iterPrioConflictExec, boolean shouldThrow) + Class exceptionClass, Executable iterPrioConflictExec, boolean checkMessage) throws Throwable { nops.attachIterator(ns, iter1); Wait.waitFor(() -> nops.listIterators(ns).containsKey(iter1.getName())); - - if (shouldThrow) { - var e = assertThrows(exceptionClass, iterPrioConflictExec); - assertTrue(e.toString().contains("iterator priority conflict")); - assertEquals(Set.of(iter1.getName()), nops.listIterators(ns).keySet()); - for (var scope : IteratorScope.values()) { - assertEquals(iter1, nops.getIteratorSetting(ns, iter1.getName(), scope)); - } - } else { - assertTrue(logsContain(List.of("iterator priority conflict"), iterPrioConflictExec)); + var e = assertThrows(exceptionClass, iterPrioConflictExec); + if (checkMessage) { + assertTrue(e.toString().contains("iterator priority conflict"), e::getMessage); + } + assertEquals(Set.of(iter1.getName()), nops.listIterators(ns).keySet()); + for (var scope : IteratorScope.values()) { + assertEquals(iter1, nops.getIteratorSetting(ns, iter1.getName(), scope)); } } private void testNamespaceIterNameConflict(String ns, - Class exceptionClass, Executable iterPrioConflictExec, boolean shouldThrow) + Class exceptionClass, Executable iterPrioConflictExec, boolean checkMessage) throws Throwable { nops.attachIterator(ns, iter1); Wait.waitFor(() -> nops.listIterators(ns).containsKey(iter1.getName())); - if (shouldThrow) { - var e = assertThrows(exceptionClass, iterPrioConflictExec); + var e = assertThrows(exceptionClass, iterPrioConflictExec); + if (checkMessage) { assertTrue(e.toString().contains("iterator name conflict")); - assertEquals(Set.of(iter1.getName()), nops.listIterators(ns).keySet()); - for (var scope : IteratorScope.values()) { - assertEquals(iter1, nops.getIteratorSetting(ns, iter1.getName(), scope)); - } - } else { - assertTrue(logsContain(List.of("iterator name conflict"), iterPrioConflictExec)); + } + assertEquals(Set.of(iter1.getName()), nops.listIterators(ns).keySet()); + for (var scope : IteratorScope.values()) { + assertEquals(iter1, nops.getIteratorSetting(ns, iter1.getName(), scope)); } } @@ -499,7 +441,7 @@ public void testDefaultIterConflict() throws Throwable { defaultIterPrioConflictVal), () -> tops.setProperty(noDefaultsTable2, defaultIterNameConflictKey, defaultIterNameConflictVal), - false); + true); // testing TableOperations.modifyProperties String defaultsTable3 = names[4]; @@ -514,7 +456,7 @@ public void testDefaultIterConflict() throws Throwable { props -> props.put(defaultIterPrioConflictKey, defaultIterPrioConflictVal)), () -> tops.modifyProperties(noDefaultsTable3, props -> props.put(defaultIterNameConflictKey, defaultIterNameConflictVal)), - false); + true); // testing NewTableConfiguration.attachIterator String defaultsTable4 = names[6]; @@ -551,7 +493,7 @@ public void testDefaultIterConflict() throws Throwable { nops.create(ns8); String noDefaultsTable8 = ns8 + "." + names[14]; tops.create(noDefaultsTable8, new NewTableConfiguration().withoutDefaults()); - testDefaultIterConflict(AccumuloException.class, () -> { + testDefaultIterConflict(IllegalStateException.class, () -> { nops.attachIterator(ns7, defaultIterPrioConflict); try (var scanner = client.createScanner(defaultsTable7)) { assertFalse(scanner.iterator().hasNext()); @@ -568,7 +510,7 @@ public void testDefaultIterConflict() throws Throwable { nops.create(ns10); String noDefaultsTable10 = ns10 + "." + names[18]; tops.create(noDefaultsTable10, new NewTableConfiguration().withoutDefaults()); - testDefaultIterConflict(AccumuloException.class, () -> { + testDefaultIterConflict(IllegalStateException.class, () -> { nops.setProperty(ns9, defaultIterPrioConflictKey, defaultIterPrioConflictVal); try (var scanner = client.createScanner(defaultsTable9)) { assertFalse(scanner.iterator().hasNext()); @@ -586,7 +528,7 @@ public void testDefaultIterConflict() throws Throwable { nops.create(ns12); String noDefaultsTable12 = ns12 + "." + names[22]; tops.create(noDefaultsTable12, new NewTableConfiguration().withoutDefaults()); - testDefaultIterConflict(AccumuloException.class, () -> { + testDefaultIterConflict(IllegalStateException.class, () -> { nops.modifyProperties(ns11, props -> props.put(defaultIterPrioConflictKey, defaultIterPrioConflictVal)); try (var scanner = client.createScanner(defaultsTable11)) { @@ -626,30 +568,23 @@ public void testDefaultIterConflict() throws Throwable { CloneConfiguration.builder() .setPropertiesToSet(Map.of(defaultIterNameConflictKey, defaultIterNameConflictVal)) .build()), - false); + true); } private void testDefaultIterConflict(Class exceptionClass, Executable defaultsTableOp1, Executable defaultsTableOp2, Executable noDefaultsTableOp1, - Executable noDefaultsTableOp2, boolean shouldThrow) throws Throwable { - if (shouldThrow) { - var e = assertThrows(exceptionClass, defaultsTableOp1); + Executable noDefaultsTableOp2, boolean checkMessage) throws Throwable { + var e = assertThrows(exceptionClass, defaultsTableOp1); + if (checkMessage) { assertTrue(e.toString().contains("conflict with default table iterator") || e.toString().contains("iterator priority conflict")); - if (defaultsTableOp2 != null) { - e = assertThrows(exceptionClass, defaultsTableOp2); + } + if (defaultsTableOp2 != null) { + e = assertThrows(exceptionClass, defaultsTableOp2); + if (checkMessage) { assertTrue(e.toString().contains("conflict with default table iterator") || e.toString().contains("iterator name conflict")); } - } else { - assertTrue( - logsContain(List.of("conflict with default table iterator", "iterator priority conflict"), - defaultsTableOp1)); - if (defaultsTableOp2 != null) { - assertTrue( - logsContain(List.of("conflict with default table iterator", "iterator name conflict"), - defaultsTableOp2)); - } } noDefaultsTableOp1.execute(); // should NOT fail @@ -759,76 +694,4 @@ private void testSameIterNoConflict(Executable addIter1Executable, addIter1Executable.execute(); addDefaultIterExecutable.execute(); } - - private static boolean logsContain(List expectedStrs, Executable exec) throws Throwable { - var timeBeforeExec = LocalDateTime.now(); - var timeBeforeExecMillis = System.currentTimeMillis(); - exec.execute(); - - // check the logs from other processes for a log that occurred after the execution and - // contains one of the expected strings - List warnLogsAfterExec = warnLogsAfter(timeBeforeExec); - for (var warnLog : warnLogsAfterExec) { - if (expectedStrs.stream().anyMatch(warnLog::contains)) { - return true; - } - } - - // check the logs from the test process (this process) for a log that occurred after the - // execution and contains one of the expected strings - return appender.events().stream() - .anyMatch(logEvent -> logEvent.getTimeMillis() > timeBeforeExecMillis && expectedStrs - .stream().anyMatch(logEvent.getMessage().getFormattedMessage()::contains)); - } - - private static String getDatePattern() { - String datePattern = null; - for (var appender : loggerConfig.getAppenders().values()) { - if (appender.getLayout() instanceof PatternLayout) { - PatternLayout layout = (PatternLayout) appender.getLayout(); - String pattern = layout.getConversionPattern(); - if (pattern.contains("%d{ISO8601}")) { - datePattern = "yyyy-MM-dd'T'HH:mm:ss,SSS"; - break; - } - } - } - assertNotNull(datePattern, - "Format of dates in log4j config has changed. This test needs to be updated"); - return datePattern; - } - - private static List warnLogsAfter(LocalDateTime timeBeforeExec) throws Exception { - var filesIter = getCluster().getFileSystem() - .listFiles(new Path(getCluster().getConfig().getLogDir().toURI()), false); - List files = new ArrayList<>(); - List lines = new ArrayList<>(); - - // get all the WARN logs in the Manager and TabletServer logs that happened after the given - // time. We only care about the Manager and TabletServer as these are the only servers that - // will check for iterator conflicts - while (filesIter.hasNext()) { - var file = filesIter.next(); - if (file.getPath().getName().matches("(Manager_|TabletServer_).+\\.out")) { - files.add(file.getPath()); - } - } - for (var path : files) { - try (var in = getCluster().getFileSystem().open(path); - BufferedReader reader = new BufferedReader(new InputStreamReader(in, UTF_8))) { - String line; - while ((line = reader.readLine()) != null) { - if (line.contains("WARN")) { - var words = line.split(" "); - if (words.length >= 1 - && LocalDateTime.parse(words[0], dateTimeFormatter).isAfter(timeBeforeExec)) { - lines.add(line); - } - } - } - } - } - - return lines; - } }