Not related to this patch, but would it make sense to rename this to something like CallbacksAndInterceptors, or something of that sort, as this is made up of user and interceptor callbacks. 
Can this just return 0 if length == 0? 
I'd rephrase that to:  ``` java LOG.warn("AMQP heartbeat interval must not be negative, using default timeout ({}).", ConnectionFactory.DEFAULT_HEARTBEAT); ``` 
Look up `Predicate<T>`, basically renders this class pointless. 
That's what I was hoping to do, but couldn't due to the storm and backtype `TopologyContext` class and how they use `MetricsDelegate`. It needs to know both the class that extends `IMetric` (T) and the type of metric (U), since it takes an `IMetric` delegate and returns the return type.  ``` public class TopologyContext ... {   ...   public <T extends IMetric<U>, U> T registerMetric(String name,                                                     T metric,                                                     int timeBucketSizeInSecs) {     MetricDelegate<T, U> d = new MetricDelegate<>(metric);     delegate.registerMetric(name, d, timeBucketSizeInSecs);     return metric;   } ```  ``` public class MetricDelegate<T extends IMetric<U>, U>     implements com.twitter.heron.api.metric.IMetric<U> {   private T delegate;    public MetricDelegate(T delegate) {     this.delegate = delegate;   }    @Override   public U getValueAndReset() {     return delegate.getValueAndReset();   } } ``` 
In the previous code, if there is an TTransportException, mIsConnected is still "false", isn't it? 
please hide the visitor implementation if not relevant for the API 
Nullability 
formatting 
Minor comment: Just curious  about motivation here to change the code? 
I was going to say we should probably break down this class rather than loading all these different properties into one place, but at this point, probably worth just tacking it on.
2015 
Because you have wrapping the passed adapter (Much like the super implementation of this method) you should also override `getAdapter()` to give back the wrapped adapter 
When does this happen? 
Add a helper method:  ``` java public String getLocalFileName() {    return (lpszLocalFileName == null) ? "" : lpszLocalFileName.getWideString(0); } ``` 
style-nit: I find it more readable when line breaks are added at higher-level breaks. Could you insert the line break after one of the commas (perhaps after the first one)? 
This is not the case, other asserts for workers.get(0).getUsedBytes() in this test later on will also fail. 
Does this work? The java:comp/ is meant to be useable within a deployment's runtime and isn't necessarily setup such that it's accessible from MSC services. 
Imho the collectPhase should be read only and not mutable.  If this doesn't work without setter then I think we should just leave it as it is instead of abusing the JobCollectContext as messenger class for the collectPhase
Why is this in the android package? I know we internally are using the Looper, but apart from that the general functionality is general-purpose? 
add a todo to use the code generator for this 
You should be building this content in the commit method, not the run method.  Otherwise, your commits are going to skip some messages. 
Is it in `contrib`? If yes - it's unclear.
stream is  a constant. Can you please make it upper case. 
Similar comment as that last one: rather than loading the typeface, this could use TypefaceCache (if TypefaceCache.getTypeface() was made public rather than protected). 
I think a better signature would be:  ``` public <V> V checkSucceeds(Callable<V> callable) ```  If this is the only change (other than the tests) could you update the commit message to be someone more specific? Perhaps "Make ErrorCollector.checkSucceeds() generic" 
You could change the name to lengthenShortUrl(). The current lengthenShortUrl() is probably not used any more. 
Indentation? Kafka uses 4 space indents for java 
a few too many spaces 
maybe consider using Completable.ambArray here 
JavaDoc? 
`final` 
![Codacy](https://www.codacy.com/assets/images/favicon.png) Issue found: [Document empty method body](https://www.codacy.com/app/torodb/stampede/pullRequest?prid=625095)
Why not eagerly create the cache here? 
I think you need to update the message to have another `{}` since `e.getMessage()` is no longer a throwable.
What's the objective here? Performance I guess? 
Similar comment about configs here as with the other Reporters. 
Collections.synchronizedSet shouldn't be needed, since installedDrivers is now a copy. OTOH, there's a pre-existing problem where the whole method should be in a synchronized (drivers) block as that field is not thread safe. 
`MaxFreeBalancer` seems to be more intuitive. 
is this intentional that newly added schemas are immediately updated as well?
nit: Indentation is a little off here. 
Should this be "cancel connect call fail" ? 
Ditto 
Why not just play() when we call this? 
Same issue on variable names. 
Heh, clearly there's no test for this check 
Likewise, please move this to the top of the test, or a @Before method. 
we might have gone beyond the endDocId in the previous loop (line:177), we should check getRight is less than endDocId every time we add to newPairs 
This whole block assumes that the same disposable instance is sent multiple times from an operation to the repository. Although I think the described behaviour is the correct one, I would assume this would probably mean the operation has failed to properly pass the ownership of the disposable, most likely leading to all sorts of trouble in other parts of the code.  It kind of make me wonder about the disposing and passing of ownership as a whole here. It does mean that clients using repo.get() should never retain that value. It does however also mean that since repo.get() can be called on any thread, there's actually no guarantee that the disposable object returned haven't been disposed of already in another thread, even if the reference is only kept on the stack?  Does this mean that disposers should only get intermediate values, and that retained values in a repository should never be considered as disposable (nor sent to the disposer)? If so it does remove some of the issues with this block, and the comment about initial value. 
I use `code` for classes that are external to Tachyon code base 
I think there is an extra space here 
Could we make a "@"link of ConsumerRecord? 
We like to make all our endpoints RESTful, so this should be `@Path("state")` and should accept the new state to transition to 
spell error 'TableScame' 
Android field naming convention done ;) 
Should be `SingleValueMap` 
Separate classes for files and directories?
why not return an empty string in the absent case (`orElse("")`) here and then not have the `@Nullable` annotation above? 
This will change it for all future requests as well. Not ideal. Should be an instance variable. Put hte context into the SocketIOConnection object. 
Let's rename this method into something like 'check...' or 'validate...' 
let me know if this is over-engineering... 
I think you could use the bytes from addBytes as argument to flush and then just use a local variable to avoid all the .get() calls.  I think the way this method currently works it isn't really thread-safe.  For example:  Thread1:   addEstimateBytesAndMaybeBreak(  flushBuffer.get()) //   5 bytes. Thread2:   flushBuffer.addAndGet (6) and gets into flush()  Thread1:   totalBytes.addAndGet( flushBuffer.get() ) // return 11 bytes Thread1:   flushBuffer.set(0); Thread2:   if (flushBuffer.get() == 0) -> returns  now totalBytes and whatever the breaker has internally is different. 
This seems an odd change. Going to switch back to being separated below. 
solve todo. imho 1 based indices like at the internal array subscript function 
Nasty, but also wonder why they decided to go this route.. it seems dumb. 
Netty makes sure not to call it concurrently, but agreed, its too hidden, will just change to atomic counters for simplicity 
I think i is not neccessary to have a `CustomCapturingMatcher`,  this implementation could be moved to `CapturingMatcher` and use an `any()`Matcher if used in a `ArgumentCaptor.capture()` call. 
`byId` here too? 
can you add comment about why the initialCapacity is 10 
Also add `!isEmpty()` just so we are nice to the compiler.  
I think this will be a bit tricky, since the current setup logic depends on the `intrinsicWidth` and `intrinsicHeight`s of the `Drawable`, which will be the original dimensions if we don't create a new bitmap. We'd also need special cases that handle the other orientations (not sure if we can mirror bitmaps in place currently without going into native stuff like JNI).  
same here 
`"Only image resources can be used with the image pipeline. " + resourceExt + " files are not supported."` 
I'm more inclined to logging to the error output stream. I believe it's more suitable for this kind of messages.  We can redirect `StatusPrinter` to `System.err`, but also should have bear in mind that `StatusPrinter` for some reason is static, so we should restore it to `System.out` after using. How about making the output stream as a field of `LoggingFactory`? Something like this:  ``` java @JsonIgnore                                          private final PrintStream configurationErrorsStream; ... @VisibleForTesting LoggingFactory(LoggerContext loggerContext, PrintStream configurationErrorsStream) {     this.loggerContext = checkNotNull(loggerContext);     this.errorsStream = checkNotNull(configurationErrorsStream); } ...  StatusPrinter.setPrintStream(configurationErrorsStream); try {     printIfErrorsOccured(loggerContext); }  finally {    StatusPrinter.setPrintStream(System.out);  } ```  As a benefit, we can pass a fake stream in the test to the class in the constructor. 
nit: sort these by the order they come from the network?  status, reason, headers, body 
} else { 
unbounded? 
Consider adding JavaDoc (and since tag).
While this is rather easy for the `Counts` class, do we really want to hardcode static query strings instead of using some helper to build them in general for the long run?
Move this down closer to where it is used 
These behaviors can be front loaded to configuration time. Another PR. 
is this behaviour correct? shouldn't we raise an error instead? 
Update the comments here. 
Should this be WARN or INFO? I feel it is not "abnormal" that the topic partition does not exist yet on bootstrap or topic update? 
Doesn't this break the semantics of Future.get() method? 
~~Why did this need to change?~~  Ignore my comment 
don't use abbreviations 
Throw a proper exception if you need something more then the interface, this will error out on class cast which isnt as useful in debug. 
nit: could you collapse all of these into fewer lines? 
Whats with the weird restructuring of imports? 
ditto 
see my comments on `ChainedDescriptor` ... I'd love it if we can make this `Descriptor<? super E>`
this should be the Alluxio type 
What happens to the vanilla fields when you add things? 
Why use this tag?
add comments before merge here please  
This is only used in one place.  Just create an anonymous class where it's used. 
I did some search and it seems the dollar sign `$` is legal to trail the user/group name. It would be good to put in a comment that which standard we are following to construct this `USER_PATTERN`
I think you can just drop the check altogether, no? 
Rather than pass in a new parameter, it seems like it would be easier to create a static method like the following:  ``` java protected static Token getStartTokenOfFirstRule(Collection<? extends Collection<Rule>> cycles) {     if (cycles == null) {         return null;     }      for (Collection<Rule> collection : cycles) {         if (collection == null) {             return null;         }          for (Rule rule : collection) {             if (rule.ast != null) {                 return rule.ast.getToken();             }         }     } } ```  You could then pass `getStartTokenOfFirstRule(cycles)` to the super constructor instead of `startTokenOfFirstRuleInCycle`. Note that the complete set of null checks in this method ensure it will never throw an exception for poorly formed input. 
Same 
Executable is a confusing suffix for this class.  Perhaps CreatorExecutor?
Code style generally favors writing out imports instead of allowing wildcards. Also, the `jopenvr` package doesn't include a domain? That's sort of odd. 
It will be great to add java doc for this as well. 
Add imports for all this nonsense 
Since Java 8 the compiler can infer such explicit type arguments, so I beleive `<JsonPrincipal>` is redundant. 
Add a comment explaining the logic here, it's a bit non intuitive. 
should this be `AccessControlException` or `IllegalArgumentException`? 
This could be simplified by using `StringUtils.stringToInt()`. 
new, not-used method 
and again... 
Nice comment. 
nit: test prefix is not needed 
normalizeIndex is used via MethodHandle in line 39. you cannot remove these
Can we add suppress warnings here `@SuppressWarnings("unchecked")` (and for the cast to List below) ?
Since the above two methods are "synchronized", let's also put this one as "synchronized" 
Put this condition outside the `inode.isDirectory` check
Any particular reason you use an `Iterator` explicitly here instead of just a "for-each" loop? 
you could just assign the condition that you use in the if statement to passed 
Couldn't we leave this as it was before? It is true that the PR removed the internal use of the method within the try-catch of `ResponseBodyEmitter#sendInternal` and that any errors from the container side should work as expected but a controller could also call `completeWithError` and the change eliminates the option to complete with error from the application side unrelated to write issues (and regression for some apps).  If a container and an application thread both try to `setErrorResult` at the same time `DeferredResult` will only accept the first. We can document to not do that after a write failure which could be one reason for a close timing. Other than application vs container side `setErrorResult` should be quite rare. If it does happen I'm not sure there is much we can do and in any case it implies the response is not usable.  
I prefer we let Js dictate the notification height by using `WRAP_CONTENT` and setting a specific height in Jsx. wdyt?
grammar: `each of them`.  On a related note, try to avoid overusing it. It is easier to understand a sentence if it uses nouns instead of pronouns. For instance, the last sentence could be `This method also requires that the output files do not exist in Tachyon as it will create an empty file for each of the output files.` 
use of "this agent" this might be copy and pasted from previous commit? 
Same here 
You have "grant_type=authorization_code" parameter in ACCESS_TOKEN_URL. looks like redundant as well 
Ditch this method completely. I just made a fairly substantial change to the API and this is a trivial adjustment to make. 
make `private`
as exception is dropped, should we at least log it ? 
typo : applicationS 
I doubt that this works in general. Can you please verify? 
Use static final instead of hard-coded values 
There are no direct tests, that's true.  It is tested through the various queries, but some direct tests would probably also be meaningful. 
Use `/** ... */` to take advantage of Javadoc tags. 
pls add javadoc
Wrong import. 
also change it in `testShowTablesFrom`` 
With all these nested `null` checks it makes me with we used `Optional` more places in our code :-/
Turn it into `new PrestoException(INVALID_FUNCTION_ARGUMENT, "a human readable string description", e)`  In general, throw this exception when input is invalid (invalid unicode, etc.). 
We don't need this constructor. It will only invite bad behavior :) 
style nit: `} catch` on same line 
Could be package private instead of protected? 
Use `ImmutableList.of(SortOrder.ASC_NULLS_LAST)` 
non-boxed `head` access 
not "any nodes" any more 
it is :) 
Was there a reason to not be consistent with the rest of the testcase -  ```         assertThat(result.isAccepted(), is(false));         assertThat(result.httpCode(), is(HttpStatus.SC_ACCEPTED));         assertThat(result.hasMessage(), is(true)); ``` 
this should be done by CommonAttributes.DEFAULT_DATASOURCE.validateAndSet(operation,model) 
shouldn't this preserve the existing type descriptor, if any?
is subclassing the best choice here? is it possible to have a OutStreamIntegrationTest base, and we fill in different write type? 
hrmmm, maybe there should be a javadoc somewhere that describes this process. There is a temp, backup, and tempbackup file, which can be confusing. 
Did you forget a git add ? 
I think we can call the options just `CreateOptions` 
Maybe simply overriding `finished` would make a more simple example? 
This comment can be removed IMO
`is` is always `null` 
If we are always dealing with character-based payloads (and maybe we aren't) then logging the decoded version of the payload in a toString() would probably be more useful than the raw byte array 
sure, can add protection. Its protected on the higher level, but makes sense to protect here as well 
Please add a `addAlertCondition()` (or similar) method to `PluginModule` like we have for alarm callbacks and others. You can then also just extend `PluginModule` and use that method in here. 
Should this be called `connectionDeadline` or something with deadline in it? I think there is a difference between a timeout and a deadline. 
(minor) I would test the positive condition `isEmpty()` and swap the cases. It is less information for the human brain to process and `!` can be overseen 
There is some duplicate code here in the if/else block. Why not move the setting of the badge number to after the if/else and do something like:  ``` SharedPreferences.Editor editor = context.getSharedPreferences("badge", Context.MODE_PRIVATE).edit(); editor.putInt("badge", badge > 0 ? badge : 0); editor.apply(); ```
should this be final? 
line break? 
throw? 
I don't think we need to have this method and the boolean flag any more. It was only necessary in the Tap framework in the old code. You also don't call it I think 
This should be `AccountPasswordMustChangeException` instead.
Should this javadoc not be moved to the AbstractMapDataStore or wherever the addTransient is defined? 
This is self-referential.  You're basically defining what an interface is used for. 
We should ideally set these threads as daemon to avoid the possibility of hanging shutdowns in clients/server. This requires associating a thread factory with the executor as we do in the server’s `KafkaScheduler`. This will also get rid of the requirement to formally close the metrics object in all the tests (although I think close is a good practice regardless if we are associating a thread pool with metrics). 
should be two space indent here and in the test. 
how much of a performance difference does this actually make? I think things are fine both ways though. 
Is it possible for `getMaxAttempts()` to be < 1 ? 
queryParameter takes the _unencoded_ value, and you're querying with the encoded value. This is incorrect. 
Can we remove the `_` at the beginning.  I know it matches the rest of the file but we are moving away from this style.
It's part of my last commit. 
This can be simplified to an `if/else` block, or ternary operator if you're feeling crazy :stuck_out_tongue_winking_eye: 
also set InstanceInfo to null here? 
Can we also check here that the annotated fields have indices that include every integer from 0 to some n? 
When I run a job from Eclipse using a LocalExecutionEnvironment, `TASK_SLOTS` is set to -1 and execution fails. This might be a problem with the LocalExecutionEnvironment, but should be resolved. 
While we're updating this, change to `RAPTOR_CORRUPT_METADATA` 
`Not Unsuspend` -> `Not unsuspend` 
Reduce the logging level to debug or trace 
When `isExpanded()` is called `isExpandable()` is checked internally too.
Kill. Use try/catch and assert something form the exception message to verify you're getting the right one 
I'm not generally a fan of the direct access to class internals approach. I would still prefer this to have getters instead and make the fields private final. 
we already did enclosing in group, I do not see reason in this wrapping one more time. Please explain a reason or remove it. 
`String` ctor takes length as the last arg, so you need to pass `limit - pos`. Also the charset is missing. 
Good find! 
invalid parameter 
Can you use FEST reflect for nicer code? 
Keeping a map of attachments to views here, to allow reliable update of views. This is transient state which will be lost during configuration changes, it's the presenter's job to keep and restore the state. 
`itterator`'s a typo 
Good point, done. 
Or just nuke this behavior! 
This could be removed. 
The runs are under the same /tmp/alluxio-tests/ directory. For example, the pull request builder might create /tmp/alluxio-tests/folderA at 1pm, then while that's running the master build starts at 1:50pm. If the threshold was under 50 minutes the master build would delete /tmp/alluxio-tests/folderA out from under the pull request builder.  Basically, we just want a safe value here such that we don't delete any directories that are actually being used. We could probably get away with lowering this to ~30 minutes, but I chose 2 hours to be completely safe. I'll change it to 1 hour since we fail the build after this amount of time anyway. 
I would say that as this class is a "common class", this utility class is requiring tests to be sure that each method is working as expected. For such class, 100% code coverage is expected 
Need a dot at the end for correct JavaDoc 
I'm not really clear why this is done in a separate loop. Are you trying to avoid the work of collecting valid records unnecessarily? Also, I think the loop below still has the OFFSET_OUT_OF_RANGE check. 
Can we try another way to do this here instead of having a default static instance reference of a child class in the TransportClientFactories interface? One suggestion could be: - define the interface for TransportClientFactories as is, but with no default static instance - have a TransportClientFactoriesProvider, e.g.   ``` java public class TransportClientFactoriesProvider implements Provider<TransportClientFactories<?>> {      private volatile TransportClientFactories<?> transportClientFactories;      public TransportClientFactoriesProvider(TransportClientFactories<?> transportClientFactories) {         this.transportClientFactories = transportClientFactories;     }      @Override     public synchronized TransportClientFactories get() {         if (transportClientFactories == null) {             transportClientFactories = new Jersey1TransportClientFactories();         }          return transportClientFactories;     } } ``` - in DiscoveryClient, initialize this provider with args.getTransportClientFactories(), then just get the actual impl out here in the same way. 
before invoking this method, do we require `parent` needs to be read-locked? If yes, we should document this
Is this only used for directory breadcrumbs? We may want to name it something else.
It's not very readable to use `%s` to format constant strings. I was suggesting that you could use `String.format` so you could use `File.separator` in the message without concatenating strings. 
abstract? 
This is really ugly. Break into two lines 
这边加上{}，提高可读性。 
`isRtl`, acronyms always get camel cased to eliminate ambiguity: https://google-styleguide.googlecode.com/svn/trunk/javaguide.html#s5.3-camel-case 
Instead of doing another map - why can't we use the Config structure?  Config auroraProperties? No need for yet another map. 
I don't think we make fields final, either, unless we need to use them in an anonymous class 
the naming is inconsistent 
Cool. 
private?
the method 
Typo, should be: "an ever-updating" counting table" 
How does the reader itself know when to checkpoint? Shouldn't the consumer decide?
I think somewhere in the parameter name should say "inquire", since this is the number of times to retry while looking for the leader. 
Why domyou remove the NPOT check? 
How about raw types? Will they fail later? 
note that `HostSelector::parse` returns `null` on bad input  I'm guessing this will yield a 500 with no error message on bad input? 
The value 64 is used in many places. Maybe we should promote it to a constant? 
This javadoc will have to be updated, I reckon. 
two space indent 
This class should now be unchanged, but here the position of import assertSame is changed and a } is deleted and immediately re-inserted. Not sure how to completely revert all changes to this file to prevent creation of an unnecessary version. 
Please set maturity to EXPERIMENTAL and severity to WARNING, for all of these checks. 
Add final modifiers to fields that are not modified.
Could we also support `io.netty.leakDetection.level`? We need to support the old name for backward compatibility, though. 
this can be private 
Why would a dimension that existed in one of the segments no longer exist in another one?  I.e. under what conditions is it expected that this will be `true`? 
Format like this:  ``` java         assertQuery("" +                 "SELECT COUNT(*) FROM " +                 "(SELECT orderkey FROM lineitem WHERE orderkey < 1000) a " +                 "JOIN " +                 "(SELECT orderkey FROM orders WHERE orderkey < 2000) b " +                 "ON NOT (a.orderkey <= b.orderkey)"); ``` 
We probably want to support NULL here. 
You don't need to add API on BroadcasterCache IMO. You can also retrieve from the Broadcaster.  The `Serializer` should be set on the `Broadcaster` IMO, not the cache. 
Add a comment here: short sleep to avoid busy looping 
You probably should make sure the `producerThread` is stopped at the end of the test. Perhaps make it a field and in `shutdown` `interrupt` and `join` the thread if it is non-null?  
This doesn't need to be a separate build rule, it can be a step in the compilation rule, at least that's how we do it in all other cases. 
nit: don't need `this` 
Why not delete this one, as well? 
Use method reference instead of lambda (see previous comments) 
Don't remember.  I'd be fine undeprecating it if you wanted to do so in this pull or elsewhere. 
Oooh, you can avoid the substring call. Push will do that for you automatically. 
minor not spelling "foloowing" 
Please check if pSecurityDescriptor could be declared to take a WinNT.SECURITY_DESCRIPTOR. I'm not sure how sensible this is, as SECURITY_DESCRIPTOR is basicly an opaque strucure, but it would be typed that way (maybe that structure needs a constructor added that takes an allocation size). 
*Instantiator 
See comment in the other file about using `final` in parameters 
You need to pass the expectedOutputs down the hierarchy to get the pruning to work properly. Otherwise, pruning will always halt right here.  planRewriter.rewrite(node.getSource(), expectedOutputs);  You don't need to worry about including node.getOutputSymbols() because by definition of DisitinctLimitNode, it will never add any new symbols from its source. 
All nodes are dependent on a camera from which something is rendered. Previously these nodes relied on an identical line placed upstream in a previous node. If a second camera was set between that node and this node, this node would render through the second camera instead.   By placing this line in this and other nodes I make the dependency on this particular camera explicit and these nodes can move in the render graph slightly more freely.   Eventually this line (and the identical ones in other nodes) will be replaced by a StateChange and/or a setCamera() method. In turn these ways to set the camera for the node will eventually take in input an entity with an attached camera component rather than an instance of a camera class. But that's still some way out. 
missing tests on UserImpl (like equals) 
This check needs changes, if the file does not exist the message is always shown (if it does not exist it should use the parent file instead). 
`isNullOrEmpty` 
we removed notions of case insensitivity in Druid, so I think this should be just 'createIndex' 
Not catching a Throwable this time? 
remove "file" 
This field definition seems strange. Why are these two interchangeable? 
Please move all `PROXY-*` properties to `ClientProperties`. We may want to re-use it also for other connectors.  Note that I am not asking you to implement the support for them in the other connectors, just to move the properties. You can add a javadoc note that these are now only supported by Jetty connector. Ideally, you can also add a  ``` // TODO Need to implement support for PROXY-* properties in other connectors ```  comment before those moved properties. 
maybe name this getValue or getNumber, since it can be used to parse things besides counts 
why don't we make it so that getIdStringQuietly() handles the 'id == NO_ID' case? 
Indentation two spaces please 
Good catch! 
This is ugly. Maybe tpch should have an enum with the logical type? 
Afaics from the code above value can never be null...
This doesn't seem to be used anywhere 
It would be nice to replace this _continue_ by wrapping the next few lines in the _if_. 
Does this need another / in http://?  Or is that already present in info.groupOwnerAddress? 
return new StringBuilder().append()....toString(); 
Remove redundant check - you also have it in JNI C++ layer. 
This branch needs to call `process(node.getValue(), context)` 
To make this simple,  ```java PropertyKey key = create(name, defaultValue); ``` It is a good practice to re-use existing factory methods/constructors as much as possible and leave only the logic that is really different here. It is easier to understand and maintain too. 
`Blocking queue to use.` 
unrelated, @Macarse has done some work to get rid of these listeners entirely using lombok.  I don't have a sense for how much performance they cost, but it seems like using `Matchers.any()` can't come cheap 
is this class needed? 
no need to do now()-based logic here, just construct url via httpl util and pass in the query params for start/end if they are present. 
is this just a pattern for clarity? because it's equivalent to synchronizing over `this` 
add a test expecting no failure
You should use try with resources here too. 
Is there a compelling reason to go with a dedicated PreferenceActivity rather than the existing PromptApnActivity?  Either is potentially fine with me, but I think we need to provide a little more context to the user about what's going on and how they should go about configuring these details than what's in this preferences.xml.  Most users aren't going to know what an MMSC URL is, so we should provide some short copy that explains what it is we need and how they should find it. 
You need to add a test where the actual mail breaks down. This should involve a mock on `JavaMailSenderImpl`. Throwing some exception on the `getSession` call would be enough to prove the exception is properly handled. If you want to do things properly, I'd test a failure on `Transport#close`. 
added 
examples package 
It seems that we only reserved 3 bits for compression codec? 
https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/TimeUnit.html#toMillis(long) ? 
Please build engine information in line with https://github.com/junit-team/junit5/issues/590#issuecomment-265621984
In this case noting should be captured, see [my comment above](https://github.com/mockito/mockito/pull/606/files#r76538541). 
I take it this starts some thread? Would it be possible to inject some executor to do this instead. I know we're trying to centralize thread creation for working with OpenShift but am not sure how strict we are about this at the moment 
nit: final 
Probably good to have  ``` java + new String(selectedProtocol, "US-ASCII") ```  since new protocols are working their way down the pipe. 
在这里加了一个判断，表示没看懂为什么要加这个判断
`@linkplain` 
I don't like the idea of injecting important behaviour through monitors. Could this not be put in the SPI.getRecoverer implementation in NeoStoreDataSource? 
please remove outer if (removable > 0) and add it here: if (removable > 0 && clockSource.elapsedMillis(bagEntry.lastAccess, now) > idleTimeout) 
I thought this generated garbage, although it being the `SpriteBatch` constructor I doubt it matters. 
put comment here `// bigger than MAX_INT`? 
We would now have a getState() and a getStatus(). I think this leads to confusion. If getStatus is a fine grained version of getState() let's find a way to communicate this fact. If getStatus() is meaningful only during the "running" state (as hinted by the EngineStatus javadoc) let's make sure that is also easy to latch on.  I thought of a few options (getRunningState(), getFineState()) but perhaps getSubState() is the most appropriate. Obviously you'd have to change EngineStatus into EngineSubState and everything associated with it. 
Builder pattern. 
Would this be easier to read if you reformulated the conditions? Essentially you're now always prepending a whitespace, except for the lastchar/',' case (where op == ',' should always be a binOp, right?). So you could just check for the binOp case for the appended whitespace. 
This if-ladder is partly why we shouldn't switch to use integer indices for these. A class or enum that can be added to a list is much better (it doesn't tie code generation as closely together with the parameters, which makes it much more extensible). 
"location" -> "index" (that's also the standard for AbstractList). Update everywhere. 
There's still a race here. Another thread might decrement `currentMemory` between the call to `addAndGet` and `get` 
Ok, I added a TODO for this. 
What other rules do we want to enforce here? 
Why is the `skewAllowance` not pass to the `GoogleAccountsService`? 
nice. 
Whitespace at the end? 
Why not simply create a custom qualifier annotation and use it here?  ``` java @Qualifier @Documented @Retention(RUNTIME) public @interface OkHttpNetworkInterceptors { } ``` 
It seems that `fetchUsers()` goes one step further than just fetching the data from the backend and continues with saving them in the DB.  I would recommend unbundling that extra step and perform it in the callback instead. That way, `fetchUsers` responsibility is kept simpler and more testable. 
This loop is extremely inefficient. Is a very busy wait loop (one CPU core will constantly burn cycles on this loop). 
Is there a need to flush the "tracer" at here before System.exit(0)? The reason I ask is I'm not sure how the automatically scoped TraceScope works with abnormal exit like System.exit(), i.e., whether span.finalize() would be called in the event of System.ext()... (My understanding is the process is terminating, there is no need to do GC and reclaim out of scope variables anymore, so the flush won't happen...).  Or we can move System.exit(0) outside the scope of TraceScope. 
For better readability please save the value to a local variable with a descriptive name first, e.g  ``` java String messageText = mMessageContentView.getText().toString(); TextBodyBuilder textBodyBuilder = new TextBodyBuilder(messageText, ...); ``` 
`BaseSpan` is a strange name. It's something like `prevSpanContext`, maybe should be a new type, which contains: traceid, parentSpanId. No need be a `span`.
The proxy can be configured on global config level too, need to use the one in the future 
I looked into this, `_score` doesn't work for either Groovy or Mvel (probably due to this being in an aggregation?). If this is something we want to support I think we should open a separate issue for it. 
Wouldn't it be better to disable NAKs as a static final and just skip the entire activation instead? @mjpt777 thoughts? 
Ouf :-) Might now want to change the formatting :-) 
same as the above... this exception should be thrown on regular Jedis class, not JedisCluster. 
fail() 
I guess these could be public final fields and lose the getters, as it's a pure data class? 
same here 
since changing it has no impact and used only in once place. I don't see any advantage of turning it into a constant really. 
This is out of the scope of this PR: we are now coupling the flushing on the store with forwarding on the processor node here, which is admittedly not ideal; and partly because of this we are adding this function only in the DSL and it from user-facing APIs. Moving forward we should consider a cleaner mechanism for allowing users to decouple flexible flushing (which is a notion of the store) with forwarding (which is a notion of the processor node). 
NIT: import order and separation:  ```java  import java.io.FileWriter; import java.io.IOException; import java.lang.reflect.Field; import java.util.HashMap; import java.util.Map;  import javax.annotation.concurrent.ThreadSafe; ```  
As discussed, lets keep this class immutable. 
I believe that on most JVM implementations, the sleep is useless. 
break line 
I'm wondering here if it'd be worth to use the BufferedImage.getData() method to obtain a Raster object and then iterate over pixels directly instead of going through integer arrays. Not quite a priority though. 
Why wouldn't this be supported? It returns a copy, which wouldn't affect the original. I think the parent implementation of this method is fine. 
why are these endpoints added under the master/block namespace? I think they should all go directly under the master namespace ... they have no relation to the block master (conceptual one anyhow) 
See my comment to `mapValues` for `KStream`.  That is, I'd clarify to sth like "transforming the value of each element in this stream into a new value in the new stream". 
See comment above please.
I dont think that this check is needed here (otherwise it will throw an index out of bounds exception, which says the same). https://github.com/libgdx/libgdx/wiki/Contributing#performance-considerations 
since the order matters for these, it would make more sense to return a List
You probably don't want or need this.  Running indexOf/substring is almost certainly faster than maintaining and looking up in a hash map.  Remember, in order to lookup in a hash map, you must call String.equals on the key which will iterate through each character of the string.  indexOf and substring will iterate through only part of the string. 
You are ignoring the return value of the latch. So you have no way of knowing if the latch.await completed successfully. Use the HazelcastTestSupport.assertOpenEventually 
If we are going to break this, can we make a accessor for this instead?   
Why renumber this error code 
I think this method can work just like `set()`. You complement the `intMask` with `~` and then `&=` it. 
separate these into two statements and remove the space before ;
"duel"? 
hm, wrong 
@DrKLO @midi We can also change this rule to: if (cs == null || cs.length() == 0 || Build.VERSION.SDK_INT >= Build.VERSION_CODES.KITKAT) {  ... and remove the other changes, so the KitKat emoji set is always used whenever available? 
Shouldn't this actually be implemented? Finite liquids CAN be filled, as long as it's the same liquid, and the the update ticks should equal themselves out. 
This was classified as a dangling comment, hence the move by a few lines and the demotion from javadoc to simple block comment. 
Would it be better to use `TimeUnit.MILLISECONDS.toMinutes()` here? 
Just wondering about if a cypher dump shouldn't also export the relevant indexes / constraints? 
If we no longer have a pool to return it to (only occurs due to `reset()` being called?), should we just treat it like a remote worker client and close it, crashing seems a bit extreme? 
Off-topic: I was not aware that Scalaz fails in that case. Do you know why? 
Naming this "resultSelector" seems off (copy/paste?) since it's more akin to `combineLatest` which uses "combineFunction". 
Is `mJournalCheckpointThread` ever not null, if it is, is it guaranteed that `mJournalCheckpointThread` is stopped?
HTTPS and the client-side certificates are two different things though, right? Having HTTPS-configured without using client-certs seems like a valid setup. 
I'm not sure if it is good practice to ignore `null` referenced streams. Why not let it fail here? We might hide other errors by skipping the problematic stream here. 
Ensure service is still valid and enabled.  
I dislike these manual `init()` and `start()`, and the fact that you return a PageCache out for which you cannot really stop the sweeper thread. I would like to have the `StandalonePageCacheFactory` perhaps be renamed and be something implementing AutoCloseable, where the page cache is created in the constructor, and it plus the lifecycled page cache added to an internal LifeSupport, which is `shutdown()` in `close()` 
Nitpick: I think it's a good practice to group all the final fields together (so that one can easily see where the mutable state is). 
+docs 
You deleted formatting whitespace before a method. Run Code->Reformat Code 
It seems character encoding is not right.  
Rename to "pool"
i purposefully made them not consistent 
"or" --> "of" 
I'm not sure how likely it would be, but should we do any string trimming on the `input` here to remove whitespace if a user passes a single space character in?  
No comma 
Can this be on one line now?
same as previous comment from `spring-test` mock. 
if the key isn't in the cache (e.g. concurrent invalidations) then this will decrease incorrectly. I think you need to check `val` and skip the locking code if not present.
can be normalized if the first arguement is a null literal 
@ajinkyakolhe112 remember to format also this file :) 
format issue, line limitation: 100 chars 
I'd prefer not to require a content type, just set it to application/binary.  Calls to setHeader on the request will respect/override the contentType appropriately. 
what would break if there was just   rand +  func_75902_a(Math.min(10, totalWeight - rand))   getting rid of the need for the accessor changes to the biome lists 
Should this be "Map operation can have only one parent"? 
ahh, this does look really fast 
I don't think we should make this change or add the additional `init` method above.  This is something that can and should be handled outside of the library.  When you're about to load the calendar, do the min/max calculation yourself, and then implement a custom `dateConfiguredListener` to handle the selectability of the dates you actually care about.  @JakeWharton thoughts?  The `containsDate`, `clearHighlightedDates`, and `scrollToDate` additional all seem fine to me. 
I think we should probably have an `Exclude` that pairs with the `Include` above. I don't know if having a bunch of well-known excludes is the way to go though. I have no better idea at this point. 
Replace DESCRIBE operation on rootRegistration with rootRegistration.registerOperationHandler(DESCRIBE, GenericSubsystemDescribeHandler.INSTANCE, GenericSubsystemDescribeHandler.INSTANCE, false, OperationEntry.EntryType.PRIVATE); 
This check would fail on itself here, am I right? 
Essentially what we do inside the ODK as well :) 
IMO this should live in the exception class. 
Why don't you check if `ifNotNullBlock` is not null ? 
I wonder, instead of taking a `CompressedObjectStragegy.CompressionStrategy`, it might be useful to have a new interface that knows how to serialize stuff?  So like a `DimensionSerializer` or even a `DimensionColumnPartSerde` that just compresses the column itself? 
Shouldn't `AppLinkNavigation.APP_LINK_NAVIGATE_OUT_EVENT_NAME` and `AppLinks.APP_LINK_NAVIGATE_IN_EVENT_NAME` be in the same class? 
Given how we are doing this in multiple places now it may make sense to create an interface to allow people to specify additional compression/decompression methods in a more generic way.  For example if someone wants to use bzip they would have to re-implement `NetworkEventReporterImpl` as well as the `RequestBodyHelper`. If we had a simple registry system from Content-Encoding to an interface that exposes a  compress and decompress function it would enable this. 
optimized slightly 
we usually use info("some log string {}", value); to avoid string concat when logging is disabled. 
I should probably have asked this in the previous review, but why "addFollowingAccounts"? We don't talk about accounts mostly in other parts of the code. It could be ambiguous with the BIP32 feature of the same name. 
I think equals is wrong here. I would recommend using `MediaTypes.typeEqual` to ignore the possible charset encoding or other possible parameters at the end of content type. 
same comment as above 
Couldn't this be: ```` uriVariables = bestPattern.matchAndExtract(lookupPath); ````
will this work in screwdriver? 
Not thread-safe, please make volatile
sgtm 
This one is not necessary, the next statement will return (without doing any I/O operation). 
This should be logged as error, if an exception gets this far it's most likely a bug in the custom menu items. 
Should this be in an internal package? I don't think it's our responsibility to expose this in a public API. 
Why are you using `isReportIterations()` instead of  `isParameterized()`? `doRunFeature` only tests for `isParameterized()` to invoke the `runParameterizedFeature` method. Why don't you just schedule every feature, otherwise we have a strong coupling to the scheduling implementation in the ParameterizedSpecRunner. 
We have to emit a `StreamsChangedEvent` here to make sure the stream router gets updated with the new default stream. This is needed because the periodical might run when the stream router has already been loaded. 
Why did you do this? Now we got a cas statement extra. We know only a single thread can touch it. So there is no need for this. 
The close needs to happen even if recordReader throws. 
Ideally, use complete sentences.  Say something like "Unresolvable devices are typically not relevant outside of scan range.  Hence, we specially clean them from the cache." 
According to current implementation of `copyMsgToBuffer`, if size of `m.getOut()` is not large enough to contain both header and data of `mResponseMessage`, then, data will be truncated, then `mResponseMessage.finishSending()` will be `false`, so `mSession.close()` won't be called.  So what will happen to remaining data in `mResponseMessage`, and if `mSession` is not closed, what will happen then? will there be another data request? but I can not find logic like that. 
This looks odd. Should just `vi>vj`, `vi<vj` and some special cases like NaN be handled instead? I do not see what `floatEquals` buys here. 
If TiledMapTileMapObject supersedes TextureMapObject, we can get rid of all this comment block too. 
Replace this entire try {} by  {Code}  try (InputStream in = new FileInputStream(this.configurationFile)) {   this.configProps.load(in); } 
We don't want to ignore this exception.  Let it fail the test if need be. 
create times => CreateTime 
It's a new type. No need to keep deprecated methods. Same applies for all methods in `v4` package. 
This is an array.
You will probably want to ensure an ItemComponent exists on this first before doing this check.  This has bit me more times than I care to admit. 
ensureYellow should be enough 
nit: `BLE` would be better, also this should be named to better distinguish it from others.  I would maybe rename `checkIfBluetoothCanAdvertise` to `hasBleAdvertiseCapability` and this to `checkAndWarnBleAdvertiseCapability` or just meld them into one method. 
I addressed this compatibility issue in  https://github.com/Netflix/feign/pull/90  @adriancole Should we just cherry pick that into master? 
would it make sense to use the Path as key directly? All usages involve `path.outputName()` and I think just looking at this class it is unclear what the key should be 
can you move the expectedException after the first `.next()` call? otherwise it doesn't verify if the first call works 
Does this need to be public? 
What are we protecting here against?  It is logical to expect that there will be some annotated glue classes when we use SpringFactory, but imagine there are some scenarios that do not depend on Spring contexts and we have SpringFactory in our class path - would not it be better for such scenarios to execute peacefully even if their glue classes are not annotated, and when they are called individually? 
The Java Validation annotations should be added to the method parameters as well so that Jersey Bean Validation could work. 
this too 
Personally I feel this API should not be exposed to the users, but only to "developers". 
We should log by using `Log.e` method call. 
is subpath a term? shouldn't this be sub-directory? 
Is it safe to use `ImmediateEventExecutor` really, for any pool implementations? 
I think we have multiple query generators as well. Is there a way to combine them into one general purpose query generator? 
I like the testing predicates! I assume that `includes(fn)` makes sure that all the contents of `fn` is included, and properly namespaced based on `fn`? 
Can you assert their type here also? 
Javadoc 
Same comment as for other similar code. 
Please flag this with a TODO to switch to prepared statements 
Can you provide some more explanation on why you think this method is required? 
I don't like the idea of deprecating this method so soon. Anyone who overrides this method in a class that extends `Theories` would likely be broken by these changes.  Instead of passing the `TestClass` instance into the `ParentRunner` constructor, why not add a static factory method to `TestClass` that has a cache mapping `Class` to `TestClass`? 
This is two hashmap lookups for each element. The check can be done only once in the open method 
Please run "Reformat Code" once (prevents unnecessary code formatting changes in the future). 
Since the AccumulatorRegistry is only used task-internally, and always retrieved form there, it should be initialized internally. Saves one more constructor parameter and helps with separation of concerns. 
"type" is just a comma delimited list of subsystems that are mounted on a hierarchy. Cgroup does not support mounting the same set of subsystems on multiple heirarchies (a.k.a mirrored hierarchies).  Thus, using the comma delimited list of subsystems as key is going to be distinct 
Our standard for enums is camel case, so `PolicyConf`. Actually, `PolicyConfigKey` would be consident with other patterns in the code (e.g., `SystemConfigKey`).
Maybe using `rotation.time.interval.ms` to indicate time unit. 
Please move the BooleanSupplier definition outside and assign it to a variable. Otherwise it's re-created for each iteration
ok so to be clear, this code deems a new supervisor "bad" at first. Then eventually graduates it to good, as long as its score doesn't get incremented.
fileswitch -> default here 
Formatting 
Only the CERT_TRUST_STATUS is embedded in the structure. The other elements are Pointers, so need to be bound as `.ByReference`
Should be `checkArgument` 
Since Proto (and Thrift) use getTtl/setTtl, our code should probably do that too. I agree with @aaudiber that it makes sense to do that in a follow up though. 
This could maybe be refactored out, it now seems to be used in about four places. 
Dont use java.util.Vector, use ArrayList 
It would be nice to centralise these numbers in a common place 
Use a constant instead.
Please remove blank line. 
Why do cleanup  based on this condition? What do you think about doing this all the time? If you do decide to keep it condition based, please add a comment here to describe why this is being done 
Add some empty lines between static and none static fields. And add space between fields and methods. 
Whats the point printing empty string to the console? 
Can we remove `jsonlite` here now that we don't require it for data preview? 
Add a TODO... this should be returned as part of the "plan" operation. 
QueryToolChest.filterSegments(..) returns an impl of this , so this needs to be an extension point.
I'm unclear here why you would call `immutableReader` if you already had a copy of the `FsChannelImmutableReader`? It seems like it is cloning the current reader and incrementing the ref on the channel reference? If that's the case, maybe `.clone()` would be a better name for it? 
Package scope? Or am I not foreseeing some use for these by plugins? 
Using a constant  ``` private static final List<Throwable> NO_VALIDATION_ERROS = Collections.emptyList(); ```  would be nice. 
As much as I don't love creating things like this, it allows us to work even when the field isn't present, yet. This is how we addressed other minor schema changes such as pre-aggregated deps and ipv6. 
please invert the condition. this is really strange to consider all loops as infinite _except_ `o.getExpression() != null` or block == nul or ... etc 
single line instead of 2 lines? 
Can you try this instead of the `getHandshakeResource`:  ``` java AtmosphereResource res = atmosphereResourceFactory.find(resource.uuid()); ```  That will saves us one lookup in the attribute's map. The uuid should be the proper one. If that works just update this patch. Thanks for all the help! 
Nice :) I like how we support different optimizations without worrying about it too much. 
can be simplified to   return searcherContext.found ? 
Nitpick: `unless a value is already associated` reads better, I think. We need to document the return value since we have now added that. Also, we should make it clear that this may or may not be atomic (as people will expect it to be atomic given `ConcurrentMap.putIfAbsent`). 
Seems like the result of this should be `"bar<7 spaces>"` not `"bar"` 
The method provides location? 
Update the log message 
nice. 
name -> ID 
Tests that pinning/unpinning ...
This can also be if (tx.getHash.equals(rejectedObjectHash)) as .equals(null) is defined to return false. 
volatile 
RESTORE_SNAPSHOT must not be allowed on write-disabled tables, pls remove
Looks like that the `len` variable is redundant. We could directly use `field.length` in the loop condition.
Does this method override a method? If so, please add @Override 
The loop can be removed now (and just replace).
is it possible that instanceState hasn't yet been set here?
It was expected that the whole collection be copied in here. In the case of the IAMCredential, for example, it will have more than one header. 
Would read better to remove the `is`. 
Please don't use wildcard imports 
NIT: can this just be called `mPipeline`?
same 
What's up with this? 
remove `this`
Maybe add that both are needed when returning this value to core. At least I was a bit confused by this. 
This should be innerHTML and not src.   Since you've got the no-callback form special-cased, you should add tests for this case in mobile-spec. Maybe verify they work by using a follow-up call that does have a return value. 
Should be this be commented out? 
Are you trying to achieve scheduling with fixed delay with this schedule-at-the-end-of-a-run construct? 
0 is not a power of 2. 
Now it is :) 
Maybe add a helper for this, since it is duplicated below 
when replacing paused boolean with finished one here, afaik finished must be set at all catch cases at innerCollect where paused was set to false... 
I would rather to have 2 different methods `getSanitizedPath` and `getSanitizedParameters`. 
We should also then adjust this to cast to map and `get("commitMeta")` 
I would like this patch a lot better if it introduced a new attribute (say "filter-spec"; better names are welcome) and deprecated the old "filter" attribute, marking it as an alternative to "filter-spec". The "filter" attribute would not actually be stored in the model -- it would be synthetic, read from and written to the filter-spec attribute. This filterToModel method would be the basis of the read-attribute handler for the deprecated "filter" attribute -- pass in the filter-spec value stored in the model and get back the legacy "filter"  complex attribute value. A reverse method would need to be written for the add handler and for write-attribute -- take a complex attribute and store it in "filter-spec" as a string.   That would be a fully backward compatible solution, and since the old "filter" is properly deprecated we could drop it in AS 8.  I'm not suggesting supporting the old attribute in the xml. If people want that they can just use the old version of the xsd. 
static import 
May be able to just use this for all -- not sure why the parameter is used to begin with. 
Changing it this way will force two new object allocations for each message (and single messages are the norm). Could we refactor this so that we don't have to do those two allocations and simply change the `ProcessBufferProcessor#onEvent` to figure out whether to invoke the `MessageFilter` more than once? 
Rename to OAuth2AuthorizationRequestRedirectFilter
why is this no longer a constructor? 
Can we validate that there are no null values 
... this is the calling code 
see https://github.com/druid-io/druid/pull/2521/files#r53891406 
wrong jsdoc here 
What are these for? We shouldn't be encouraging people to serialize these instances. 
static import 
should we emit some kind of warning here? 
Please mark the method also as @Deprecated... Not only in the documentation....
no need for local vars here, just use values on stmt 
sure. 
there's some formatting problems in the PR 
`Displays information for all files and directories under the specified path recursively.` 
same 
the naming is inconsistent 
Extra space. 
Needs `@since 1.4`. 
Maybe extract the `0.5f` into a constant called `FLOAT_FUDGE` or something like that, with a comment explaining why this helps?  Same thing for the `- 2` below? 
Could ppDacl and ppSacl replaced with WinNT.ACL? 
Why not use [`TransactionStatus#canRollback()`](https://docs.jboss.org/hibernate/orm/5.0/javadocs/org/hibernate/resource/transaction/spi/TransactionStatus.html#canRollback--) here? 
That description is kind of loose. What about the dreaded comma vs dot issue, for example? Also who writes currency amounts in scientific notation? 
can we add a note about what is for backwards compatibility? 
``` final StatsProvider statsProvider = getOptionalStringArg(cmdline, "pd")   .transform(new Function<String, StatsProvider>() {     @override      public StatsProvider apply(String name) {       return ReflectionUtils.newInstance(name.get(), StatsProvider.class);     }     }).or(new NullStatsProvider()); ``` 
The modified files had too many warnings, reduced and extracted them to simplify the review of next commit 
setter could be made Nullable as limit() on the CollectNode can be Null anyway. So you can avoid the null check 
`Gets` 
Don't leave this empty - add call to (implicit) _super()_ as this enables setting a breakpoint in the constructor 
Reverse: TestNG is actual, then expected (JUnit is the opposite). 
inline this method, since it just throws and exception. You can remove the comment too 
This won't work - the function is declared as:  ``` c Status XGetWMProtocols(Display *display, Window w, Atom     **protocols_return, int *count_return);  ```  protocols_return is a pointer to an array of atoms (following the documentation). The array of atoms is allocated on the callee side and not on the java side. As an Atom is basicly a primitive long, I only see the option to bind this as a PointerByReference. From that you have to extract the data itself and in the end free the list:  ``` java         X11.Atom[] atomList = new X11.Atom[count.getValue()];         for(int i = count.getValue() - 1; i >= 0; i--) {             atomList[i] = new X11.Atom(protocols.getValue().getLong(X11.Atom.SIZE * i));         }         X11.INSTANCE.XFree(protocols.getValue()); ``` 
`originalTimestamp` can be null here, causing an NPE 
Do we have a sense of why this file lock is still held?  Even if this check is bad because it can't be truly reliable, IIRC in the past this test didn't fail every time, so I fear we broke something. 
Please add Apache license header
Should not it throw on a null callable? What's the point of calling with null? 
I like the idea of having multiple adapters but instead of another wrapper adapter implementation, I'd introduce a new adapter method in Logger ``` Logger.init()   .addLogAdapter(new AndroidLogAdapter())   .addLogAdapter(new AndroidFileLogAdapter()) ```  This would be more intuitive. We can still keep the old one as deprecated to provide backward compability and put deprecated annotation
The cache control headers are to be inserted ONLY IF the filter is rendering the response, not if the filter is forwarding along the chain. You may end up setting cache headers on assets among other things significantly killing performance both for clients and the server. 
the consistency check level to apply to this property
the [0] here seems volatile 
init the variable at the variable declaration. 
I don't think this is needed, since you should not able to create a table unless the table currently does not exist and in that cause you could not have partition actions. 
Do these really need to be moved?  It is cleaner to have them grouped nicely, however that's a formatting change and should be included as a separate PR. https://github.com/zaproxy/zaproxy/wiki/DevGuidelines#formatting
Add a comment about what this is testing. From what I can tell it is that a in-memory realm is not fully deleted until all threads has released their instance. 
I don't think this should be changed. Moreover, this is named wrong already. The boolean signals whether or not an image was loaded from cache. 
This code can be simplified, I think. If the capping of playbackPositionUs to be within bounds is done before the relativePlaybackPositionUs calculation- at line 782, some of the if/else checks that follows can be avoided.  Then Util.binarySearchFloor can be used in both cases.  If the position has reached (beyond) the end, and isReset is false, chunkIndex = mediaPlaylist.segments.size() can be returned, to keep the current method return value behaviour.  
Needs a test. 
you can just declare the test as `throws Exception`
Extra space on this line. 
Could we simplify this with https://commons.apache.org/proper/commons-lang/apidocs/org/apache/commons/lang3/builder/ToStringBuilder.html? 
No it's not the same ;) 
Your commit message disagrees with this method name. Is that because `doOnCompleted` just mutates the `Observable` rather than actually subscribing? 
can't this be wrapped by a transport exception if this ends up being a remote operation? mayne use the new `assertThrows` on `RestStatus.NOT_FOUND`? 
Could you please remove this empty line? 
consider using an enum instead of boolean as return value for better readability 
The `@return` clause mentions nodes, but this is a relationship. This comment is also inconsistent with the other two in that it has no `<b>Note</b>:` part. 
Use _assertEquals_ in case the non-zero value can help diagnose why the test failed. 
formatting seems off 
make this some nice and round number. E.g. `65_536`. Preferably expressed as 0xFFFF 
I don't understand this comment, especially when looking at the code.  I realize that this is somewhat copypasta, but it should still make sense, and I don't think it does right now. 
Can you add some javadoc? 
Should this have a null check like `getValue(int, String)`? 
do you plan to put this use of TimeUtils back in at some point? Might make sense to remove TimeUtils from this PR until you actually want to use it. 
This could fail if args[0] is accidentally something not cast-able to a long. I would suggest just passing `args` to the `Raw` class and keeping the existing args parsing and error handling logic that was in place in the `Raw` class. 
This should use a ServletContextListener to do the uninject 
same as the above comment. 
Might be worth noting what needs synchronization (accessed by heartbeat thread and user thread) and what is safe. I was having trouble sorting out which methods need synchronization and which ones don't. Seems like the shared state is `heartbeat`, `state`, `client`, `time`, `groupId`, `retryBackoffMs` (where `time`, `groupId`, `retryBackoffMs` are threadsafe) 
`@see #setOnBlah` 
Your implementation doesn't account for callbacks which have a return value. This was mentioned in the original issue as something we should reject in the parsing part. 
Would be great to have some tests that exercise some batched announcements and then unannouncements and reannouncements and stuff like that. 
I think most of these import (or variable) test cases should be moved to a test for the pre-processor. This way the current tests focusses purely on actual parsing and the pre-processor test focusses on pre-processing. 
nit: formatting 
Can this be private? It only seems to be used internally. 
This shouldn't have changed. 
with slf4j use paramerized logging http://slf4j.org/faq.html#logging_performance 
For the javadoc I would copy the javadoc from executor.execute:  ``` RejectedExecutionException - if this task cannot be accepted for execution. ``` 
Layer violation. 
the result of plan is already casted to NonDistributedGroupBy, so this check is redundant 
Ok, we really need to extract this dirty piece of code into a separate class 
Usually we don't return handlers (because the user code knows what was set). Is there a reason for this one to be present?
Inline 
After checking with @StefanRRichter, it seems this check is unnecessary anyway since `org.apache.flink.core.memory.MemoryUtils#UNSAFE` is getting the class if needed and if not existing, Flink should not start anyway.
needs test 
Slightly confusing that `force` does not let you do this.
is the syncing recursive?
Use `ByteUnit`? 
but we're still not getting e root cause , no ? 
This test is not necessary. 
whereArgs is never null, isn't it? 
I'm not really happy with this API, open to suggestions 
Indentation 
Rather than providing overrideable ways to set the client, maybe we can add the necessary constructors to EurekaBootstrap:  ``` public EurekaBootstrap(ApplicationInfoManager infoManager, EurekaClient client) {     this... = ... }  public EurekaBootstrap() {     this(null, null); }  ``` 
Is there a reason we don't `lock.countDown()` in here? 
Or, would this be more appropriate if it were called "prepareUfsFilePath"? 
same thing here... but it can go in the outer `finally` block, just after `catch (InterruptedException e1)` 
Not project style for spaces, should be:  }<space>else if((headers == null && headersPos != 0) || (headers != null && headersPos == 0))<space>{   Easiest thing is to look at the pre-existing style in the file and just copy that :) 
No, this is only used for connections right now. 
Ha! Yes it should :-) 
We need to check if `mScreencastDispatcher` is non-null and not overwrite it with a new reference. Otherwise we could have 2 of these pushing screencasts to two separate clients, and one of them will never ever get a `stopScreencast()` call. 
same here 
I am wondering would it be better to have this logic in `kafka.Kafka.main()` instead of in the exception?   Also it seems that after calling shtudownSystem() the thread will continue to run in this case, which is different from the previous behavior, i.e. system.exit() does not return. If we do so we need to inspect all the cases where we call `System.exit()` and make sure all the threads will exit after we change it to throw the exception.  By putting this logic in `kafka.Kafka.main()`, we can still call System.exit() in any of the Kafka thread after catching the `FatalExitError` exception and it is guaranteed the thread won't run anymore. 
Do we need equals / hashCode? This doesn't seem like the type of class you'd use in a collection, or that you would be comparing. I don't have a great sense of when is appropriate to implement these, but I prefer to leave these out until it's clear how and why they are needed. 
same here, use the backwards compat method 
`Set` + `LinkedHashSet`? 
nit: Can you fix alignment? I know the convention here is a little annoying to follow, but it does make it easy to read the schemas. 
you need to bindTo here also 
Parameter should be called `siteId` rather than `sideId`. 
yes it did, updated it to just ignore the null 
I'll merge this now as it fixes the breakage I introduced, would you mind adding tests in a follow up PR though? 
final 
Just call `toString()`. 
same here 
I found this line elegant but also confusing to read. Maybe a more imperative version would improve readability (and also improve perf a bit).
What calls this? 
Same comment as above. 
Throw an `Error` if you don't expect this to fail. Otherwise it should be handled properly. e.g: - Unwrap `InvocationTargetException` and rethrow the cause. - Throw an `Error` for other impossible exceptions such as `IllegalAccessException` and `InstantiationException`. 
This is simple but I wonder if there’s a datastructure better suited to this. I’m thinking about Android and the amount of boxing and unboxing that happens here.  (I guess we shouldn’t worry about micro-optimizations until we know the code is working!) 
I think adding a Rectangle class to the API is confusing. Users don't need any new feature besides the position/width so it shouldn't be in the API. The union is used internally by the `PreviewModelImpl` and can be called in a different way than by subclassing Rectangle. 
I think we need to make sure `getRole()` isn't null here, otherwise there could be a NPE. 
Close the realm. Do it with a try-finally. See notification tests 
This is new code that didn't exist in storm proper.  I think it would be cleaner to have this be a debug message instead of an info message.  Also could you use "{}" substitution instead of +?  ```         LOG.info("Zookeeper state update: {}, {}, {}", ZkKeeperStates.getStateName(state),  ZkEventTypes.getStateName(type), path); ``` 
it's more similar to the rest this way 
Can you delete this constant? 
check hash functions in guava here?  http://docs.guava-libraries.googlecode.com/git-history/master/javadoc/com/google/common/hash/Hashing.html I assume we don't need heavy-weight hash functions but choices like CRC might be sufficient. 
Make this a constant as well, so we always return a constant string (seems better than creating a new string each time) 
Is this not needed? 
receive - > emit? 
why a copy? 
No need to fix this right away. But "backend" code referencing "UI" code is always an error. In this case `MessageCryptoSplitter` and friends should probably not live in the UI package. If there's a real dependency on some UI-related code there's an abstraction missing. 
Is this branch being tested by a unit test?
`nextFlush != null` was way more clear to me than `indentLevel != -1`. I can't tell you how long I spent staring at the `indentLevel != -1` looking confused at what it was doing.
change to BROWSER 
Let's put the code in the matching package structure, in this case, we could use "com.baeldung.chartoarray"
Should indicate that this is executor service will use daemon threads? 
typo: `chack` 
no raw types pls .-. 
Why you decided to go with Rx here? `executeAsBlocking()` will be faster! 
private final 
It's best to do an `isFinishing()` check here, ie:  ``` if (!isFinishing) {    refreshDetails() } ```  It doesn't happen often, but it's possible for the callback to fire after the activity is finished, resulting in a crash if you try to access its UI. The `isFinishing()` check avoid this. This is one of the benefits of the `EventBus`. 
Can probably remove this comment since #290 is addressing this as part of KAFKA-1843. 
s/Number/number 
is this commenting style OK with Presto @dain ? 
in beforeTest() reset()  is a wipeDataDirectories() call.. I think that should do that already. 
I think you can make this much simpler by using `Arrays.asList(elements)` 
I think there is a race condition here. If the app crashes before here, we'll have two jobs with the same singleId. Is ugly but i guess we'll have to make it a transaction in the persistentQueue such that cancellation of the old one (if exists) and the new job should be done together. (haven't checked below yet so ignore if you are already handling it). 
maybe we should remove these assertions from all tests beside SQLResponseTest or another IT ? 
I'm probably missing something subtle here but in the previous test the pipe character was specifically used, suggesting it's the correct separator. This test seems to suggest it's among the wrong choices.... 
Again I think all of these should be debug messages, we don't need them printed out all the time, but we can configure them on if we are doing debugging. 
Comments need updated, now generated by the server. 
maybe wrap inside a `isWarnEnabled` block to avoid varargs object array allocations
Same as above.
Delete. 
Here and elsewhere can we throw the original exception and not wrap in RuntimeException?
nit: space before `:` 
why srcInode's lastmodificationtime not get changed here? 
What is a static initializer method?  I think I intended the `staticInitializerBlocks` field to hold this info but never got around to implementing
Can you fix the indentation? 
PMD dislikes this as well 😉  . 
http://developer.android.com/reference/android/text/format/Formatter.html#formatFileSize(android.content.Context,%20long) 
How about using `Collections.newSetFromMap`? http://docs.oracle.com/javase/6/docs/api/java/util/Collections.html#newSetFromMap%28java.util.Map%29 
Can this just be a Runnable? 
there is a static notNull predicate in guava 
seriously? 
The registered service is put into the context here, only if a service is extracted.  
double space here 
Add a comment like this:  `// TODO: this is a temporary hack that should be removed when the new planner is introduced.`  (@martint is currently working revamping our new planner. It will take a while though.) 
Could this result in other possible exceptions thrown in KafkaConsumer.poll / commit ? 
this comment is confusing, when testing local there will only be one worker ... get rid of this 
Perhaps use `otrClient.getDisplayId()` directly in `getString` since this variable is not used elsewhere...?
Indentation looks wonky. 
Unused import.
Ditto 
I'm not quite sure the latter two methods should be in `ResultSubpartition` now since they are quite internal. `increaseBuffersInBacklog()` is only called by `PipelinedSubpartition` and `SpillableSubpartition`. `decreaseBuffersInBacklog()` is (additionally) only by spilled/spillable subpartition views and therefore could be package-private in `SpillableSubpartition` only.
Where is the streams being used?
These javadocs are formatted horridly - remember that its primary purpose is to render in an emitter. 
Please fix the indentation here 
Shouldn't you be getting this option in a way similar to this?  ``` java SharedPreferences prefs = context.getSharedPreferences(PushPlugin.COM_ADOBE_PHONEGAP_PUSH, Context.MODE_PRIVATE); boolean force_show = prefs.getBoolean(FORCE_SHOW, false); ```  Accessing FORCE_SHOW directly is only targeting the `String` defined in PushConstants, and a `String` is always true'ish, thus making it not an option to decide on. 
This has to be consistent with `EQUAL`. However, `EQUAL` changes its behavior based on padding option. We may want to just remove that config flag 
This should also be updated depending on the outcome of the naming discussion. 
https://bugs.eclipse.org/bugs/show_bug.cgi?id=365704 is no longer enough, as setting eclipseLink.target-server to "jboss", will only cover lookup of the TransactionManager, but I don't think that will enable this TransactionSynchronizationRegistry workaround.  
albeit unlikely the middle bits will throw, I'd add a try-finally. Also note server-modules are language level 8, so you can do a lambda 
I would prefer not to use null ... just a empty set  and then assert that its not null 
consider just having two constructors in lieu of the builder. 
Should we use ATOMIC_MOVE here? 
revert 
This case is strange. I would just throw an NPE. For a varargs to be null the user would have had to explicitly call it with `(String[]) null`. 
@rculbertson I get -1 on my boot2docker version: v1.3.2, Git commit: e41a9ae. executionDriver() returns "native-0.2"  What version of boot2docker are you using? 
Would it be possible to use one of the tables defined in T3 or TableDefinitions?
You probably want a lower-case `boolean` here (`Boolean` is a boxed type and is nullable)  
This looks more like a debug statement to me. 
Calling process.getInputStream() gives you the stream to read data sent to stdout by child process. 
up a line 
can we make this into an enum that has a method fromString(String ordering)? 
ditto 
I think we should offer default implementations of these three methods as well:  getCacheStrategy() => null, preMergeQueryDecoration() => runner postMergeQueryDecoration() => runner 
Should it not include interacting e.g. Flint and Steel? 
Not used 
It should be used 20, right? (That was the default value used when reading from configuration file.)
this must be true by default. 
why is this different to the one used in the SelectAnalysis? 
Insert one space before the opening curly brace (please also fix the other occurrences) 
no need for final here btw :-) 
Missing license header 
Is \z some special token for new lines on any platform? I have not seen this before? 
In retrospect, reseting the task here was probably a mistake since we're not actually in the group yet, right? 
Same here
I would add this class to `MetadataEqualsHashCodeTest` 
Maybe add `@Nullable` for this method?
sort the setters?
Done. 
Is it possible for passwd to be null, if tokenList is the correct size? 
It's unsafe to cast your List to ArrayList.  If you change Notification.generateEmptyList(..) realization it's possible to get ClassCastException Use new ArrayList(notifications); 
This way it looks better :s 
I think you shouldn't clone the body of the method and you should use the method `callSuperMethod` from `APTCodeModelHelper` ([here](https://github.com/excilys/androidannotations/blob/develop/AndroidAnnotations/androidannotations/src/main/java/org/androidannotations/helper/APTCodeModelHelper.java#L171:L185)) instead. Because we are not sure that the annotation will be processed if the content of the method changes. 
can be static
make them final 
I'd add a javadoc comment here so it is more apparent the returned instance uses the identity function for comparison. 
I also removed this additional level of indirection. Please, review that its ok. 
"Sum" and "+1" does not go together... Should it be "count"? 
remove whitespaces with `String.trim()` before checking for `length() == 0`. 
You can change these explicit asserts into `buf()` calls that stores the result to a local variable. 
Add a comment on the next line about intentionally falling through to the next switch case 
' .. because the I/O thread has been interrupted.  Use shutdown() to shut the NioSelector down.' 
I'd remove the word FBOBuilder. 
dig. 
I don't think this is strictly necessary. It's one of those things a human writes because it knows `Thing.Thang` carries semantic value whereas a soulless (killing?) machine doesn't need it. 
If someone overflows a long, we'll all be retired or dead by the time they come to report the problem. 
This is probably just OCD but you could do a `for` loop an have less duplication. I think that would be a lot cleaner. 
Use `java.lang.String#format(java.lang.String, java.lang.Object...)`, something like :  ``` java String.format("%4d. %s %s %n", x++, ". Unused... ", m.getKey().getLocation()) ```  `4` is hard coded here, but that would work up to 9999 hint. It's still possible to compute that value if hard coded value don't work. 
This can also be deleted, the selection already displays the alert.
same here, could use the `FutureActionListener`
Perhaps a small description saying that this test is nondeterministic and depends on the GC doing it's job at the wrong time. Right now it is not all clear what exactly could go wrong here. The description for this PR might be shortened and used as description as it is very clear about what is wrong  
Make use of the HazelcastTestSupport.getOperationService method which returns the InternalOperationService. 
It seems character encoding is not right.  
Is this method meant to be overridden or accessed by subclasses? How about making it final (and also private if possible)? 
Can you make this a interface?  You can move the fields to the local implementation and don't need the static constructor for now. 
Unnecessary 
Is this used? 
These could be proper javadocs 
nit: revert 
do u still want to retry? 
fix import order 
provide exception description 
Are the \n because the sql formatter inserts them? It'd be more robust to test that the queries are syntactically equivalent, instead (by comparing their ASTs) 
`IOException` is not thrown here, so this is redundant. 
![Codacy](https://www.codacy.com/assets/images/favicon.png) Issue found: [Position literals first in String comparisons](https://www.codacy.com/app/checkstyle/checkstyle/file/3961453439/issues/source?bid=2193633&fileBranchId=3722095#l556) 
Why do we need empty constructor?
Is the factory class implemented already? If not, please add a TODO here. Similar for the provider. 
Please format and remove spaces
Probably should be `OrgLoader` based on changes made elsewhere. 
Could we use a shorten version `Validation.buildDefaultValidatorFactory().getValidator()`? The Hibernate provider will be loaded dynamically and we don't test validation of `Optional` values. 
nifty 
Add a variable for `HdfsEnvironment` 
This field can be final. 
more elegant and less lines == we won 
Space between words and equals sign 
It's nice to have them in the interface so that users can work with `FileSystem`s instead of needing to use an abstract or concrete class to have access to convenience methods. 
Again, I'm hesitant to change something like this on a point release. We don't recommend running `/hystrix.stream` through zuul. 
Leaving out the last placeholder (`{}`) in the message would print the exception's stack trace instead of just calling the `toString()` method: http://www.slf4j.org/faq.html#paramException 
It's missing the `@Override` annotation (same for other `getValue` methods).
Don't leave this empty - add call to (implicit) _super()_ as this enables setting a breakpoint in the constructor 
Please add { } even if its a single line statement. 
why is this reordered?
This cast is also unsafe. Tuple2 is final, therefore we don't need to use Object here I think. 
This IOException is swallowed either, should we re-throw it?
 - The null check can be removed (already done earlier).  - Mixed indentation chars.
So this is the only new code (non conversion of spaces to tabs).  Basically just use one gesture detector for both fling remove and longpress detection. 
Redundant method 
For windowed store's cache, since we control the serialization format of the windowed key, could we consider using a more efficient comparator than the default `Arrays.equals` as in `Bytes`? For example, I think we only need to compare the bits for the timestamp fields, and treat all records with the same timestamp as the same (i.e. no ordering) since we know in windowed store we are always iterating over the whole window? 
final  
Shouldn't this be a 407? 
`return javaSymbolMap.values().contains(type)` ? 
Why do you think these lines are not executed? 
Seem the above two lines have indentation problem? 
AsyncTaskLoader should be imported from the support lib for compatibility. 
We should simply change the `Sorter` constructor to throw a `NullPointerException` if you pass in a `null` comparator. 
Before we were using `maxBlockId + 1` for the block id, but this changes it to `block.getBlockId()` in `readRemotely`. Is this intentional? How does this test pass? 
you really use java.lang.Error for that? While it is a problem which is similar to noclassdeferror, i find that pretty hard. done anyway :) 
This is used in only one place. I don't think you need a variable for it. 
Typo. 
pls make this method static by passing in the initialSettings
import order is not right:  http://tachyon-project.org/master/Startup-Tasks-for-New-Contributors.html http://tachyon-project.org/master/resources/order.importorder 
Please rename to `intValue()`. 
We may want to keep first refactor the aggregator querying code for use to query metrics, and just convert these tests to metrics?
make the separator `,` a constant
Wouldn't it be possible to return early here and just use an empty assignmentList?  I'm not sure if it makes sense to have the verification further below that global settings don't conflict with session settings 
If I understand correctly, this code only runs when the app is opened, not when the push message arrives (That's what happens on my Device, anyway). If this is indeed the case, it can't update the counter badge until the app opened, which doesn't solve the bug - no?  
Maybe something like `setPdfViewBackgroundColor(background)` instead? setBackgroundPDF isn't very intuitive. 
It would be good to have javadocs for added methods 
BTW, I added constructors to ModelNode so now you can just say "new ModelNode(false)" 
Why do we explicitly remove children tickets? Most of the time, it's service tickets: aren't they removed at service ticket validation? 
`AlluxioMaster master = new AlluxioMaster(new MasterContext(new MasterSource))`? 
See other comment 
Same with this -- it ends up calling scheduleSplitWorker under the covers 
This error number conflicts with the error numbers in #697. Since those error codes shipped in the 4.4 release of the optimized fork, it would be helpful if `NONCONFORMING_LR_RULE` was renumbered as error 169 (leaving space for 165 through 168 to be added to this project when #697 is ready). 
The responsibility of this class seems to be to deal with job (un)deploys. Can we call it `Jobs` instead of generic `Utils`? 
Here and the previous lines you can remove some since for DS we only have --- Problems for relative address to root ["data-source" => "*"]: Missing attributes in current: []; missing in legacy [elytron-enabled, credential-reference, authentication-context] 
Use `StandardCharsets.UTF_8` 
is `-1` used because there is no tier associated with the under storage? Could you set it to be a constant? Its unfortunate there is no invalid value for the tier, ideally we should just leave the tier field unset in this case (similarly for sessionid).
just check size of partitionedByColumns 
For consistency should use `onAttachedToWindow()` 
Can we call this html for continuity.  
empty line 
Shouldn't this also be updated to reflect the new changes? 
Why do we throw here, but not in `get()` 
perhaps I'm daft, how do we handle this case now? 
There are some additional corner cases to test. What if collection null. What if argument <=0. Etc etc. I'm sure you can come up with some additional cases. 
I'd prefer "-robolectric-" over ".robolectric-", more maveney. 
Actually, to have better portability, could we use `tachyon.util.io.PathUtils#concatPath` for this path calculation? 
The code was written in that way because the files will be located under the given path directory and `cloudBlobDirectory.listBlobs()` can return files (`CloudBlob`) or virtual directories (`CloudBlobDirectory`).  I'll change the code to get a flat list of all files under the given path and then delete each one of them. If no file was deleted, a warn log message will be filed. 
This comment is outdated now that this is in a separate method. Are we only ever allowed to drainBuffer() as part of resume or if we are not currently paused? Can we add a comment here to details these restrictions? 
Is the Kafka coding convention to do braces around single-line if statements? Because I've noticed them being skipped before, but I didn't speak up because I didn't know what your conventions are. 
comment on the sort order.
Do all databases support multiple column matching in IN expressions, like `WHERE (A, B) IN (select A1, B1 from ...)`? Or at least the ones that support this new strategy.  This could also be covered in tests, where currently a single-column PK is used.
@cberner  How about to replace "your query" with the specific query information(id, context, etc)?  It might be more meaningful (helpful) for the people who submitted the query.   Another question is:  if this "biggestQuery" has higher priority (e.g. we want to sacrifice other smaller queries to get the result of "biggestQuery" first),  is there any config or option? Thanks. 
can't we log and return null here as well instead of relying on ExecutionException occurence to print this message?
So this may be me misunderstanding, but putting the the `.indexShardStateChanged` call outside the mutex in these places looks like you could possibly get state changes out of order. Is there (or should there be) any order guarantee for `.indexShardStateChanged`? 
this is weird to hardcode `System.out` 
`assertThat(map).isEmpty()` 
Maybe make this abstract? or move the implementation from DBWrapper to DB? 
nit: space after `if` 
What calls this? What happens if the file is already complete?  What does "reinitialize" blocks mean? 
This shouldn't have changed. 
rename this `hdfsTemporaryDirectory` 
Delete this test. 
I think you can create the callback once in the open method and then pass the instance to all async calls. This way, you save a lot of instance creations 
Please add check for exception cause type and message, we must be sure that it's really expected exception. 
Checks for different NBT tags here 
Rename the variable to `mNonPersisted` and change the comment to:  `A list of non-persisted inodes encountered by the traversal.`  The reason for this is that the traverse function does not need to know that the inodes are to be persisted. It simply identifies the ones that are not persisted, which leaves the purpose of using this information up to the caller. 
It should be closing container, not Jedis connection. 
We don't use import *. This shouldn't have changed since our code style sets "class count to use with import *" to 9999. 
Following the standards agreed a while back, parameters should be final and variables don't have to be. 
- nit: We put `=` at the end. - How about using `/** ... */`? 
... Edges. \n (right now it\s .,) Vertices.... 
Looking at https://www.postgresql.org/docs/current/static/protocol-flow.html  Terminate there is no mentioning of the server having to send a closeComplete message?
it seems done is set to true in all cases and this will never be false. 
this should imho only be done in the if and else if block, otherwise bulk operations will generate mutliple job entries 
better to explain it. for example, you can simple add "(e.g., 777 for rwxrwxrwx)" after "digital representation" 
do we need copy in method name ? isAvailable() ? 
Please fix this codacy issue
Maybe better to revert the condition? If logging is bootstrapped, we can return early. Advantage is a reduction of a big _if_ block with identation. 
"Add" -> "Adds" 
Any specific reason for not using just the String literal here. As in just using String avroConfig; instead of creating String object. 
Revert 
The spacing is wrong, please reformat the changes 
Does not really throw `NoSuchEnvironmentException` is it. There's also a race condition here. Between the time you check `hasEnvironmentNamed` and call `environments.named`, some other thread could have added/removed the environment.  IMO, it's probably best to delegate straight to `environments.named` instead. 
I think you are not using this at all. 
as exception is dropped, should we at least log it ? 
done 
In the `AbstractTicketRegistry` in the `core-tickets` module,  the `getProxiedTicketInstance` method builds the delegators and we need specific ones for the OAuth tickets. 
FYI, I will probably leave these as `@GwtIncompatible`, since they're used only from other `@GwtIncompatible` code. (We might as well spare the GWT compiler the trouble of compiling and discarding them, and we might as well give ourselves the option of adding non-GWT-compatible code to them in the future.) I'll update the comment, though. 
same here 
`{@code ..}` 
same here 
Again, I don't think this is safe during concurrent writes 
Yes, this should be `static final`, updated. 
RR -> round robin 
Don't add a new method. Instead, add the parameter to the existing one.  Alternatively, if this is really only for testing, it should go in the test code, but I don't see a reason to do that here. 
why are we not using jackson library
I agree, I'll remove this and force the caller to specify which breaker. 
We could import `ChannelFlushPromiseNotifier.FlushCheckpoint` to make this shorter. 
I know. But this method is also being called from the outside (backup operation if I remember correctly) where there is no response object. In the future I want to get rid of the response objects completely so that when a response-packet is retrieved, direcly from the IO thread we can deal with the handling instead of having that response-thread. 
Any plans when to release v2? 
Ah, this must be the reason.   Let's not do this, this will cause confusion for a normal behavior. 
@srinivasupadhya @pshivana I do not see tests for the behaviours of `RunMultipleInstance.java`. Is it written somewhere else? Even `InstanceFactoryTest` when run with coverage did not touch any line in `RunMultipleInstance`. Am I missing something? 
Is this correct? Doesn't the `Host` header have the following format and cannot be parsed by `URI.create()`?  ``` Host = "Host" ":" host [ ":" port ] ; Section 3.2.2 ``` 
Do we need validation for this constraint? 
not a big fan either, but note we do use `convertToStringListIfBytesRefList` in two places. So I moved them to the builder instead. 
Same as before: can we get this from the `SocialProvider` enum? 
Also not clear why "numSegments - 1" here. 
can we add a serde test checking for this? 
Hmm, this breaks once we implement another `AuthenticationToken` which returns non-null in `#getCredentials()`.  What's the reason for the type check here?
What's leader in this context? The herder?  
Doesn't this mutate the position of the buf? (I hate ByteBuffers for this reason :-P) 
How to do you signify to retry forever? `-1`? 
Technically, any argument could be a ConnectorSession 
can we encapsulate this code in a method that does the delete and logging? 
https://github.com/Bukkit/Bukkit/pull/879/files#r6378930 
This logic makes sense, but what doesn't is what `currentTime` is expected to be after the loop on line 225. Because it gets reset on line 213, after the loop it seems to represent the time after the last tuple was deserialized but before it was handled. This seems really arbitrary. If that's really the intent could you add some comments below to clarify the time math that is occurring after the loop with `currentTime - startOfCycle - instanceExecuteBatchTime.toNanos()`.
JavaDoc 
nit: we can close the output stream and the input stream separately, so this could be checkOutNotClosed 
Can you please remove the extra space after `=` from here (while you're changing this file). 
must have -> must be
This might be more readable and succinct with `&&` chaining, what do you think?  ``` java return timestamp == create.timestamp &&     ttl == create.ttl &&     version == create.version &&     id.equals(create.id) &&     type.equals(create.type) &&     ... etc ... ``` 
this should track outstanding demand and fail if there is none 
nullifying the serverChannel is meaningless now I think, so no need for the final clause. Can we open an iterator, and use it to _remove_ the relevant channel, and wrap the close method in a try catch blog with debug log on failures for each channel? 
yea, I am conflicted about the right default here, will put it in the comments for people to chime in 
sure I'll add the check to validate 
do we need to reset this after this integration tests to avoid polluting the system properties of following tests 
Does it mean there is always in memory override registry? 
do we really need both `i` and `nextModeIndex` ?  Seems like just tracking `nextModeIndex` would be sufficient. 
Missing RealmList<Simple> from this I think 
Hmm... can we trim the garbage by just looking for this method (i.e. trim until we find `Task.getCurrentStack()`)?  That seems like it'll be a little more future-proof and will work regardless of whether we're in Android or Java. 
nitpick but name again, `invalidateOptionsMenu` should be sufficient, no reason to have implementation details of method in method name. 
Change to XXHash64 
It will be `called` after Java Compiler (looks like I made same mistake in javadoc of sqlite-ap) :) 
it's ok to pass null to `getTypePresentation` 
maybe "Insert column name '%s' is specified more than once" 
? element.getName()? 
All conditional logic such as this needs to go away. You want to look up the relevant statement from a cache based on the storage name and/or ticket type rather than hardcoding checks. You are not just dealing with TGTs and STs and checking for types manually does not simply scale. 
small nit, missing a space after `+`
When would `relChainPosition` be `null`? 
Actually `e.toString()` is usually short (typically one line), callstack info is not in it 
`checkNonNegative` or `checkZeroOrPositive`? 
This is "deprecated since" notion, so usually it would be 3.0.0. But since we're breaking it straight away, it doesn't much matter. :) 
I know, that this call is technically OK - it will work, but it does not really feel right.   Actually the entire class again shares a huge amount of code with InjectLinkFieldDescriptor. I think it would really make sense to extract the common parts into abstract superclass or do some other refactoring that reduces the amount of repeating code. 
InvalidPathException 
@Scottmitch I don't know if I agree or disagree. I can write a test that involves HttpObjectDecoder to demonstrate the problem and how this class solves it but EmbeddedChannel seems to be adequate for it. `RecvByteBufAllocator` deals with bytes read from the socket. This class deals with messages that were produced by upstream handlers. How these messages came to life doesn't matter. Some messages such as `LastHttpContent#EMPTY_LAST_CONTENT` are not even backed by actual bytes. What matters is that handlers are shoveling messages downstream and this class gives the user a tool to control that flow and builds on top of the existing API.  
"fails" -> "failed" 
when replacing paused boolean with finished one here, afaik finished must be set at all catch cases at innerCollect where paused was set to false... 
@szpak Do we need this class? An other option is to provide an addtional constructor in AssertionMatcher with description parameter. 
loader --> classLoader  Dagger has loaders, they are different things. 
Java 6 compiler is not smart enough to infer `Integer`. 
Remember to change `givenDummyWithId` to match with the new naming. 
hyper nit: `String.format()` reads better than `+` 
same comments as in GroupByQueryQueryToolChest.
Is this correct behavior? Eureka source seems to indicate that leaving on the port as a part of the vipAddress is standard:  From AbstractInstanceConfig.java:  ``` java     @Override     public String getVirtualHostName() {         return (getHostName(false) + ":" + getNonSecurePort());     } ```  Seems to me that honoring exactly what's passed in by the user (minus http/https) is preferable. 
I don't think this is pronounced `an R-eactive Streams`. 
Rename to "pool"
getDisablingTask/getEnablingTask to be removed. 
This looks like an Eclipse code reformat. We should also avoid this sort of change (except in commits dedicated to reformatting or whitespace) to avoid the noisy diff issue Kevin mentioned) 
Not that it matters here, but could also be `ifPresent(Consumer)` instead of `isPresent`/`get`.
whitespace 
Perhaps we should do this as the `DELETE` verb of http.
does this have to be `path.getPath()`? createDirectory() uses `getPath()`. 
You get the idea.
I prefer the was the code was
You can delete both of these try/catch blocks. If an exception is thrown it'll fail the test. 
It's a little hard to tell what the test is testing, since it's all contained in a helper method. I would suggest removing the TODOs at the end of the method and in-lining `test_dispatchKeyEvents`. 
`final` 
Why do we change this to `void`, it seems like in most cases we directly try to `get` afterward. 
ditto
include patterns were wrong if called via aggregated `main` method :/ 
Comment block should include info about the response message(s). 
`resultFlag`? 
What about pulling this out of the if/else entirely? Then the else has append("Preconditions") and below has .append(replacementMethod).append('(') for both cases?
Javadocs 
Why not an AtomicLongFieldUpdater? Now you create more litter than needs to be. 
It would be good to add a message here, "Expressions can only be used for search scripts" or something similar. 
that is a copy/paste error from my side, it shouldn't cause any problems. let me fix that. 
Test this for index 2 and 3 as well (should be 2=empty and 3=null) 
In the testing we use mostly the following naming practice:  test_shortConverted_whenIllegalArgument shortConverted_whenIllegalArgument or  test_shortConverted_whenIllegalFormat_thenNumberFormatException shortConverted_whenIllegalFormat_thenNumberFormatException.  
Would it work to move the exception message to a String var just above here? So much noise. 
I wonder if the Wrapper shouldn't have a close() method if its own instead of making the people who use them know about all the things stored inside and what should and should not be closed. 
check `if (listener != null)` before calling listener method? 
Needs exc 
right, missed it.. 
Don't need this instanceof check here. Other code will validate the config for schema errors.  
Should we use primitive `int` instead of object? 
Naming suggestion: `shouldReturnNotValidIfTheItemIsNotPersisted` 
consider `Iterables.getOnlyElement` instead of `get(0)` to make the assumption about the number of arguments explicit.
This block could be less confusing if it were not nested.  if (empty) { ... } else if (size == 1) { ... } else { ... } 
Was this removed because it wasn't used anywhere? What if there are users depending on it for some reason? 
Do we want to remove the serialVersionUID ?
same as above 
This should use: `KeycloakUriBuilder.fromUri(ResolveRelative.resolveRelativeUri(requestUri, client.getRootUrl(), client.getBaseUrl())).build()` 
Shouldn't this be applied to link, instead of ichat? 
will there be duplication in the case when this returns an InputStream ... which is partially read and then there is a temporary network glitch and consequently an IOException. parent class does the retry (which starts reading data from beginning again) and ends up introducing duplicates ?
It would be much better to replace this `if` clause with a JUnit assumption instead e.g.  ``` Assume.assumeFalse(sLocalTachyonCluster.isDummyUnderFS()); ```  Just returning makes the test as passed which isn't really accurate because they are not being run.  Using a JUnit assumption means that JUnit will mark the test as skipped if the assumption fails which gives a more accurate picture of which tests were actually run 
This operation (or the flush below) might take a while and doesn't need to be in the synchronized block? 
目前DL对主题相关的支持有要求，首先host的application、DLProxyActivity、DLProxyFragmentActivity均不能有主题，host中其他activity可以有主题。插件中所有的activity都可以有主题，但是只能是系统主题（经过测试，发现系统透明主题也不支持），目前DL暂时不支持插件的自定义主题，不过，自定义主题可以通过系统主题+style来代替。注：如果不按这个来操作，将会导致三星手机上DL无法正常工作。  你这种方法经过兼容性验证没？ 
Same comment here about the mocking 
It might be worth dumping this in the Presto wiki to make it more discoverable. 
why not put this in initializeListAdapter? 
why 5 ? 
Done 
No ss. 
doesn't match current style.  Can you revert? 
Where's the implementation for this method? Was it supposed to be in this pull request? 
might want to use createReference and createFunction from TestingHelpers 
What if 't' isn't in the capacity array?
There's a flaw with the way this is implemented: if the number of pending loads is < batch size and batchLoadIntervalMillis hasn't elapsed yet and reload is never called again (or for a long time), any pending key will be left in limbo until someone calls reload again. 
Is the `queryParam != null` part here redundant?
base_request null check is missing. Also, this is different from the code in Jetty6AssetsContextHandler. Why is that? 
some comments on interface explaining the purpose would be nice 
`NoopNamedSource`? Also move to the bottom so the tests are more apparent. 
why is this check inside the loop? 
I was going to say we should probably break down this class rather than loading all these different properties into one place, but at this point, probably worth just tacking it on.
done 
i could be wrong, but it looks like the finished boolean variable is in lockstep with the inMemoryExchange.isFinishing(), which begs the question of whether you need this variable or not. 
In the model I think we use the singular form? 
TODO的这个说明正是开源需要的，将下面为什么 上，左，右 置位0提供说明，与覆盖的case。 
rename to match Java naming. 
I believe you can remove the last 'in.' 
I think `Stack` is one of those "not recommended" classes, quoting:  > A more complete and consistent set of LIFO stack operations is provided by the Deque interface and its implementations, which should be used in preference to this class.   Stack is, IIRC, synchronized and eg. `ArrayDeque` probably not, but from what I understood (took a quick glance only, though) there is no need for it here. And if there was, using `Stack` wouldn't really close the window of opportunity for a race condition. 
Preconditions are expensive, lets remove them. 
`!isReadable()` 
I guess that one slipped through. 
Return object set only if the type is not a primitive as you did in another pull request. 
why do we need to deploy a verticle here ? to get a new context ?
not really important, but we could probably rename this to WildFlyRaDeployer & AbstractWildFlyRaDeployer 
HttpSolrClient and CloudSolrClient both extend SolrClient. Can you just store a SolrClient instance, instantiate it with either HttpSolrClient or CloudSolrClient? This way you can remove all the `if cloudMode do this else that lines` and do the action regardless on the SolrClient instance.  If there are places you need to distinguish you can cast. 
Will fix that. 
Please javadoc this class to explain what this Evictor is, especially what PartialLRU is given people usually understand what LRU is. 
Perhaps specify that this will effect all loggers?  `Sets the current LogLevel. Setting this will affect all registered loggers.` ? 
This should actually be `"(" + process(node.getValue(), null) + " IS NULL)"`. It needs to be parseable sql 
should there be OR instead in: `maxEntriesBeforeSpill == 0 && memoryLimitBeforeSpill.toBytes() == 0`  if either is set to zero, we should always use in memory builder, right? 
It has to be final for the anonymous inner class, but moving it down there makes a lot of sense. 
if we make the changes I suggested with regards to the aggregator, I think we can avoid needing the getMetricClass() function and keep the original logic 
line 88 - 90, is it only for debug? this may really affect performance as even debug is not on for LOG, the while loop is still executed. 
`Pair.create` 
Copyright shouldn't be removed. 
Name this `expectedSql` 
Should this be called from Bus.unregister()? 
because the projectorChain.startProjections takes the context as argument and outside of the if/else block the projectorChain isn't available 
this would be a bit easier to read if you used `System.out.format` and a format string instead
package private? 
`AtomicInteger` ?Use the constant defined earlier?
Avoid star imports 
I would change this to contain a single "__" 
It may be worthwhile to find if any of the Request.Options can be translated to Ribbon's HttpRequest.overrideConfig. 
Drop `IcsToast.` so it's just `TAG`. 
can we add a test for this behavior? 
All "deleted" lines from here to the end of the file were just moved with a TAB to fit the IF indentation. 
do the similar in-place edit for compilerList 
Sure. Since it isn't relevant for performance I picked one. I'm fine with the other. 
remove `public`? (All the constructors in this file for non-public static classes are marked as public).  
It's not in 'core' module now. And I think, "async" is not the valuable keyword already. So as for me the package name should be just com.github.scribejava.httpclient.ahc to reflect maven arifact id 
This is not part of this pull request so it is confusing to have this comment be a part of this class.  We may want to change the comment until this functionality is actually a part of storm.  
nit, prefer this in a named subclass or a member variable for improved fluency:  ``` mChromePeerManager.setListener(mPeerListener); ``` 
This seems kind of indirect. Isn't this equivalent to setting the min version for each API to 0? Is the null check needed?  
this can be final 
I'm sure there's a reason. But, why logger.error? Why even log it to the logger, if it's in the console? To be able to correlate? 
Typo: "Makes given account keys follow", not "to follow" 
This can also be `Class<T>` which avoids the need to cast every time 
is it really required to loop over the results? if size() > 0  you could set the exception directly. 
:+1: on `503` 
remove comment - public constructor doesn't prevent instantiation. 
This and the following should be `@PublicEvolving`
Why do you decide here if security is enabled based on the COOKIE's existence. I think it could happen that users torn security off, but still have some cookie set. 
Have we identified others than HTC One X? If so we just list that and mention we have only heard it for that device. 
can we call this `maxPossibleBuckets` ?
found this bug, too. 
Nit: Rename to something more precise, like refreshAssignedPartitionsIfNeeded
done 
Note: this is a public API change, but that's the consequence of exposing a dependencies types. This will have to be a Wire 2.0 if we adhere to semantic versioning. 
personal preference: `private static final float CENTER = 0.5f;` 
Get rid of the null queue check; don't allow for null collection. 
Do we need to throw some exception here if interrupted?  
This was a bug, but for whatever reason we weren't suffering it. 
JIRA is created and javadoc is updated. 
Currently there are too many different logical assertions in this single unit test. This breaks the "only one assertion" rule. (see http://xp123.com/articles/3a-arrange-act-assert/).  Moreover, the combination of `asList` and `I.of` is very confusing.  Please think about applying the following renamings: - Change name of private class `I` to `IterableFactory` - Add factory method `IterableFactory.listOf(Object... elements)` - Add factory method `IterableFactory.setOf(Object... elements)` - Add factory method `IterableFactory.arrayOf(Object... elements)`  This also makes it more obvious, that the type of the `Iterable` is not taken into account. 
I think that this technically violates the method spec, but I don't think it matters. 
Why pass these in the context in addition to having setters on the plugin? 
This key seems weird. Why not derive it from the value so its more deterministic what the key will be? 
Obviously, a bit outside the scope of this diff, but why can't we just reuse `SymlinkTree` here?  The only different seems that we want to create the links via walking the deps, but this looks like it could easily be done in a method that does the walk to build up the link map, then just news up a `SymlinkTree`. 
Is throwing an exception really the best way to communicate that shutdown didn't succeed?  I might just not completely understand why a shutdown could fail, but it seems like it could be a normal thing? 
What is your reason for no longer adding this dependency? 
"DataFileChannelV2"
It tells about opposite thing, unify please `This intention moves struct field assignment to struct initialization block.` 
looks like result is never initialized? 
Should this produce `application/octet-stream` instead of `application/json`? 
volatile/unused 
This isn't true. See `RestAdapter.Builder#setExecutors`. 
let's use {} syntax for consistency 
use optional equals 
👍
In `PutObjectStub` you could also check that otherwise `internal()` was never call.  Is it also possible for delete? 
The getter/setter descriptions are way too repetitive and the boolean ones ("gets if the contents..." / "sets if the contents...") are just weird.  A better solution is to simply remove the description everywhere:  ```java // getter /**  * @return Whether the content is updated at the same time as the box.  */  // setter /*  * @param value Whether the content should be updated at the same time as the box.  */ ```  (If Checkstyle complains about this, make it shut up by setting `allowMissingPropertyJavadoc` to `false` in [our config](https://github.com/MovingBlocks/TeraConfig)) 
Is this field needed to reproduce the issue? 
Can you please revert irrelevant changes like comment/code formatting? It's harder to see what happened with all this diff. You could also squash your PR branch. 
This is going to break if we add `int` as a native container type, so I would put this in an `else if` and only do it when the native container type is a reference type (other than `Slice`) 
final? 
Is concurrency control important here? There is still no concurrency guard against two threads that execute in the following order: Thread1________Thread2 S1 -> S2_______ ______________S2 -> S3 ______________onNext S3 onNext S2             
For the time being it is ok, but in the future having control on the executor is the better choice. I you are going to process hard core operations on the same executor that are used for e.g. async call completion, these calls can be delayed (and even timeout due to no heartbeats being send since the operation has completed and the invocation is without response)
we are not heathens 
Should this only be `android_resource` deps like we filter out `javaDeps` above? 
The same is 0 for a primitive long value 
The convention for methods annotated with `@BeforeClass` is to name them `beforeClass` 
Personally I would not do a size check. Since it is a readonly operation; the size should not change... although of course it is a bit fuzzy. 
while this does not change functionality, it is bad style. If anyone later changes stepiterator to support removal, we'd have to remember chaning here as well. So I'd say keep the previous approach for clarity. 
This really should be static, the columns are the same for all the models.
nit: no need to alias the containing scope. Just use `getInstrumention()` rather than `me.getInstrumentation()`. If you really want to be explicit, you can use `CordovaPluginTest.this.getInstrumentation()` 
it appears the first element is something special. The thing I'm a bit worried about in terms of code clarity. If I am writing a new accumulator, what do I do with first? 
This is very highlevel, but what do you think about following the fielddata pattern of `.load()` and `.loadDirect()` for this? I think it might be nice to standardize even if it's not through a single interface. 
NAME should not be in this list. 
set the type when you declare the queue 
Can we just return `AsciiString` here? Then we won't need `isEqualsIgnoreCase()` below. 
what does this do?
This should be removed since we changed the implementation of `validate` and the implementations of `config()` in subclasses can be simplified to use a `private static final ConfigDef` of their own. 
This currently clashes with https://github.com/Graylog2/graylog2-server/pull/1613, which one do we de first? 
I think this won't get executed if the execute before throws the exception 
This version of quote() writes directly to the Appendable, avoids another buffer copy. 
Can we instead of adding this code use a TestScheduler?  https://github.com/ReactiveX/RxJava/wiki/Scheduler#testing-schedulers 
I like the idea to condense the tests into 2 methods.
javadoc parameter `user`? 
In line with previous comment, this should be renamed to "StateChange". 
nit. `"" + command` can be replaced with `command`.  
What was the purpose for initializing these values? 
Use @Nullable annotation on return - it documents to users that this may be missing 
not sure if you still need to merrge testConf back to WorkerContext.getConf(). 
nit: `test` prefix is not needed 
I see your point, Bobby, and I also can confirm the behavior you described.  Right now the behavior is as follows.  If the maximum number of reconnection attempts is reached, we enable the flag `closing`.  One effect of this is that any subsequent `send()`ing of messages will result in those messages being discarded.  Example log message:  ``` 2015-02-16 10:06:29 b.s.m.n.Client [ERROR] discarding 1 messages because the Netty client to Netty-Client-supervisor4/10.0.0.104:6701 is being closed ```  A second effect is that the Netty client will never attempt to reconnect again (by design).  But this has a negative effect, too, and I think this is what Bobby is alluding to.  If a Netty client's `closing` is true (= because max reconnects was reached), then a Storm task using this Netty client will never be able again to send a message to the target remote destination of the Netty client.  Only if the Storm task itself were to die will such a reconnect be possible because we would then also start a new Netty client.  The current code will not, as Bobby is pointing out, cause the parent Storm task (or even the Netty client) to die -- instead the client will stay forever in the `closing` state, and the parent Storm task will continue to call the client's `send()` method for new messages, which in turn will forever drop any such messages.  Off the hip I'd say there are at least three approaches for addressing this: 1. Let the Netty client die if max retries is reached, so that the Storm task has the chance to re-create a client and thus break out of the client's discard-messages-forever state. 2. Let the Storm task that uses the Netty client die if (one of its possibly many) Netty clients dies, so that by restarting the task we'll also get a new Netty client. 3. Remove the max retries semantics as well as the corresponding setting from Storm's configuration.  Here, a Netty client will continue to reconnect to a remote destination forever.  The possible negative impact of these reconnects (e.g. number of TCP connection attempts in a cluster) are kept in check by our exponential backoff policy for such connection retries.  My personal comments to these three approaches: - I do not like (1) because I feel it introduces potentially confusing semantics: We keep having a max retries setting, but it is not really a hard limit anymore.  It rather becomes a "max retries until we recreate a Netty client", and would also reset any exponential backoff strategy of the "previous" Netty client instance (cf. `StormBoundedExponentialBackoffRetry`).  If we do want such resets (but I don't think we do at this point), then a cleaner approach would be to implement such resetting inside the retry policy (again, cf. `StormBoundedExponentialBackoffRetry`). - I do not like (2) because a single "bad" Netty client would be able to take down a Storm task, which among other things would also impact any other, working Netty clients of the Storm task. - Option (3) seems a reasonable approach, although it breaks backwards compatibility with regard to Storm's configuration (because we'd now ignore `storm.messaging.netty.max_retries`).  Personally, I am fine with either a separate JIRA or, if consensus is reached quickly here, to address this directly in this pull request.  If we want to address this directly in the pull request, we only need to change a single line in `Client.java` (apart from follow-up changes such as updating `conf/defaults.yaml` to remove `storm.messaging.netty.max_retries`):  ``` java     private boolean reconnectingAllowed() {         // BEFORE:         // return !closing && connectionAttempts.get() <= (maxReconnectionAttempts + 1);         return !closing;     } ``` 
NIT: alluxio convention for private constructor ```java private PropertyDocGeneration() {} // prevent instantiation ```
Construct the object once? 
@Nehon it's an example of using Enum.valueOf(index);
xor. love it. 
Why commented out? Add `@Ignore` if not used or broken. 
This one we need a comment on too indicating that this is from System.currentTimeMillis and not Simulated Time.  Not sure if we can use Simulated Time here or not.  If we can it makes things simpler. 
same applies here 
nice test! 
added 
Yeah, but it it was failing the build when I making the change. Seems to be fixed by https://github.com/netty/netty/commit/526dafca7505904f26460a46cbe0283e161a1e9a 
`expected` or `ignored` 
![Codacy](https://www.codacy.com/assets/images/favicon.png) Issue found: [This class has too many methods, consider refactoring it.](https://www.codacy.com/app/mmoayyed/cas/file/2749420172/issues/source?bid=3256115&fileBranchId=3357337#l64) 
why is this method needed? can we use polymorphic deserialization instead? 
added a TODO.  In general would it work if we switch to a different format for entries, like proto? We can't add methods to proto classes... 
don't cast 
No braces here please. 
Do you look at the diff when after you commit? 
do we need all of  binder.bind(.).in(ManageLifecycle.class) and LifecycleModule.register(binder, KafkaExtractionManager.class); and @ManageLifecycle at KafkaExtractionManager ? I believe either one of those should suffice. 
We already have this as `MorePaths.TO_PATH`. 
A fragment should not return its views back to the parent activity. If anything you want to retrieve the Fingerprint object. 
cast to object for sure? if so, maybe better to define point as object, afaik no Double[] required.. 
`s/and timezone//`
Is "a loop" a cycle or that a node has more than one "parent"? I think the terminology may be wrong here.  i.e.,  this has a loop/cycle:  ``` a -> b  b -> c  c -> a ```  but this does not (it's a DAG but not a tree):  ``` a -> b b -> c a -> c ``` 
Should JavaPoet detect and log a warning or throw when you do this? 
I tried the break the wheel there by avoiding numeric names, but using self explanatory names.  It seems you are not letting me to :)
nit: I'm seeing this all over the diff: why 8-space continuation indents? should be 4 spaces
and here
Why is this a `Long` instead of `long` given that we assume it not to be null below? 
Please add Apache license header
Can we put semi on the same line? -> `};` 
As per Bin's comments, I think this class should also be marked as not thread safe. 
Incomplete sentence "If the connection hostname is not explicitly". 
i don't think all this should be in here, showing the thumbnail is probably enough 
I would replace the lock and condition with a `CountDownLatch` (initialize it to 1). Then in `onShutdown` call `countDown()` and in `waitForShutdownComplete` just wait on the latch 
@mttkay should we define what kinds of things will go into the sample so that we have a cohesive sample application? I wonder if having some sort of "theme" around the sample app would be a good idea? 
This is a bizarre method. Nobody in Java wants a method that returns false if it didn't do what you asked it to. That's File.mkdir() ridiculousness.  I think you might want to just set the size and then enqueue a worker to do any necessary trimming later:   executorService.submit(cleanupCallable); 
please check the access-level (the latter two could be private)
It's not about ensuring anything, it's about actually doing it. It should probably be "generateApiClass()" or "includeApiClass()" or something that shows direct action. 
This exception type doesn't make sense - it isn't an illegal argument exception (not is the one above it) 
Javadoc string missing for `name`. 
I dont suggest this modified code, 
Do we need this constructor?
Copy/paste typo. 
Could use `Lists2.copyAndReplace` which is a bit more efficient.
This is missing `smallint` and `tinyint` 
One last comment - could we cache the instance, instead of creating a new one every time? 
This range (and with the a 1.25 falloff value), will make a point light source act in roughly the same radius as a placed light.  Before (placed torch): ![terasology-160406170308-1280x720](https://cloud.githubusercontent.com/assets/5922109/14396044/3bd14340-fd8a-11e5-9370-68c88e10f1f9.jpg)  Before (held torch): ![terasology-160406170314-1280x720](https://cloud.githubusercontent.com/assets/5922109/14396052/41aa5f0e-fd8a-11e5-8de1-569a1c220161.jpg)  After (placed torch): ![terasology-160408093441-1280x720](https://cloud.githubusercontent.com/assets/5922109/14396062/485541e8-fd8a-11e5-9b6a-cacf50ddea4f.jpg)  After (held torch): ![terasology-160408093448-1280x720](https://cloud.githubusercontent.com/assets/5922109/14396072/50065544-fd8a-11e5-868a-7a0e1437743f.jpg) 
You should check whether the global entity already exists, or set it to not persist. 
two-space indent 
Do we know whether these headers will be provided to image requests resulting from this url? 
Can we use `testNewClientConnection` here instead of the two statements above? Should we do the same in other similar places? 
I'd focus in **how** you can achieve a similar effect (e.g. setting the system property) and not in what the method used to do. 
Just to follow common coding style can you add brackets here and below? We usually don't use bracketless if statements.
Here, the new resolved name is shorter and  still valid. 
This should be able to work with both HttpHandler and HandlerWrapper instances 
why "only one" and not "at least one"? 
would it make sense to use a NoopUfsDeleter if `replayed` is true?
May as well merge this into `collectStats` above, save the inlining cost. 
I decided to add the tests to ShadowViewTest, since this is for a hidden class. When I did this I was able to expose the underlying bug, i.e: that View.setRotation(10f); View.getRotation() != 10f on API 21+ (but it passes on API < 21) and I like this better as i was testing the Android API.  https://github.com/robolectric/robolectric/pull/2309/files 
Missing period. The types should also be wrapped in `{@code }` 
Construct the object once? 
s/Validate/Validates
:bulb: 'cannot' is one word (also relevant to the messages for errors 172 and 173) 
also redundant in case of the CollectionTerminatedException 
Does this need to be non-private? 
not need for -> no need for 
is binder.bind(..) necessary given that you are doing LifecycleModule.register(..) ? 
Maybe just do the test in all cases, but move the `ìf``in front of``fail``instead. 
why do you want to start all threads at the same time? 
we put static imports before non-static ones. See https://google-styleguide.googlecode.com/svn/trunk/javaguide.html  when in doubt, don't reorder imports. 
This variable isn't used and can be dropped. 
get the bootstrap from the injector and call stop 
Why did you choose UK as default? :)
Does Google need to retain a by name copyright here?  
Ditto
commented out code 
Wouldn't it be better to have either this code as a ctor argument or, because there are already so many artuments, create builder for this class? 
would read better to squash all these nested if statements into one with `&&`s 
Do we really need to have finals in interface definitions? Having finals on local variables I already find questionable due to horizontal noise.. but on interfaces... 
for larger data sets with many dimensions, does this introduce any non trivial memory pressure? 
Is there a reason to have both of these? 
header 
forgot to remove the sys out? 
might be better to use specific class instead of *
remove the comment?
We should get rid of this print logic. If we do logging, it should be through the logger. 
i don't think sparkFillPaint needs a stroke width, it'll always be hidden by the stroke drawn by the sparkLinePaint. we should probably reset the stroke width to zero in the constructor as well, since we're copying values over when setting up the sparkFillPaint
![Codacy](https://www.codacy.com/assets/images/favicon.png) Issue found: [Avoid unused imports such as 'android.util.Log'](https://www.codacy.com/app/triplez/open-event-android/file/2406672459/issues/source?bid=3233850&fileBranchId=3233902#l4) 
Need a `<p>` for rendered output to start a new paragraph. 
Missing `final`. Also, typo in `cachListType`. Please use `pocketQuery` instead of `pq`. 
what is the use case where one needs a child context, but does not need a child span?   
comment to indicate why different by factor of 5? 
So should this be _numSnapshotWindows * 2?
What's that?
I see you actually included the tests, do you mind adding the @should annotation and use the test generator plugin if you can figure out how to use it 
wrong test name :) 
nice 
If `blocksCloned` ends up being 0, should another message pop up here? (e.g. No blocks were cloned)
Same as above :-)
![Codacy](https://www.codacy.com/assets/images/favicon.png) Issue found: [Use equals() to compare object references.](https://www.codacy.com/app/cgeo/cgeo/file/4914681880/issues/source?bid=3217135&fileBranchId=4063303#l98)
same as above, `Worker` is also a `Disposable` in 3.0.4 and `shutdown` is deprecated. So I'd recommend to implement `dispose` and `isDispose` there too.
Note that 'null' should be 'nonce' here 
Perhaps change the method name to something like setTierSls
This interface has gotten complex. We should think about this as a generic schema registry interface irrespective of the underlying implementation. All these points may not be related directly to this patch - 1.  It is odd that the register API takes in some forwarding agent. This is entirely an implementation detail of our schema registry that got leaked here 2. isCompatible is actually a legitimate API that belongs to a generic schema registry interface 3. None of the *Master or *Identity() APIs seem like good fits for the interface. These are also leaking the implementation details. 
rogue log statement
I feel like this does too many rounds of `analyze()` and too many rounds of `getFresh()`, but that's just a feeling, I've got nothing to back it up. Will investigate. 
Missing `final` 
There is a small dichotomy here...   `EmbeddedCacheManager` is an interface and for remote one we use `RemoteCacheContainer`. I think it would make sense to rename `RemoteCacheContainer` -> `RemoteCacheManager` and `RemoteCacheManager` -> `Default?RemoteCacheManager`. If you won't do it now - we'll stick to this dichotomy for whole ISPN 9 time frame. 
final can be removed 
Update the documentation here. 
Same here:  ``` java  public LynxConfig setFilterTraceLevel(TraceLevel filterTraceLevel) {     if(filterTraceLevel == null){         throw new IllegalArgumentException("You can't use a null instance as filterTraceLevel");     } }  ``` 
The scope parameter MUST be present, and contain openid as a scope. Otherwise the behavior is unmodified 
obvious comment, remove 
Hi, closing the random access file will also close its channel, that we continue to use. Did you run honest profiler after applying this patch? 
I don't think this should be started until onTrigger so the ProcessSessionFactory can be set. Otherwise, the channel could start sending events to this source, which will immediately fail because there is no session. 
extra comma 
![Codacy](https://www.codacy.com/assets/images/favicon.png) Issue found: [Use explicit scoping instead of the default package private level](https://www.codacy.com/app/keith-yokoma/droidkaigi2016/file/4593894447/issues/source?bid=3216850&fileBranchId=3951353#l41)
its bad form in maven to have more than 1 main per module. perhaps just keep the code but comment it out? 
unused cast ;) 
This is done at the time of the final setup test now and hence only once, since if the libs folder is recreated for some reason, the tessdata folder is left untouched. But I admit: the situation when running directly from the Maven stuff is not covered yet. I will finally check and decide it. 
Like this
Perhaps TLongObjectMap could be better here?
Do you fancy O(n<sup>2</sup>) traversal with removal? :) I was about to suggest filter-converter dropping the keys before I've realized that you need the keys for effective removals.
Indentation is spaces here when it should be tabs. 
Can you revert these? It's a bit of a noisy diff.
Shouldn't we change to `Log.w` also here?
Voor hele file: fix whitespace (spatie vóór '{', extra newlines weg). 
We can't check that in. :)
why have you changed formatting? 
Minor comment: could be static to signal, that we don't access object members (so really save to use in constructor). 
Yes it was intentional. So the rest of the code doesn't attempt to execute.  
Change this line 
what is the reason we don't use reflection free by default? so TP run faster. 
Please add spaces around `?` and `:`. 
E.g. this block, couldn't we extract something like `statementFactory = buildStatementFactory()` which would then encapsulate the entire logic for building the SF?
Typo: The majuscule is missing.
Backwards incompatible change. 
Will this cause getCause() to be called twice on an exception since the code inside the try block also calls getCause()? 
could you rename this to storelogimages? 
There is a getActiveOrders() that you can call and simplify this code
nit: put it between static method addEmojis and the class
How about making this static? That'd make it clear that this won't modify `this.mappings` - at which point we could name the `map` parameter `mappings`, which makes more sense IMO. 
Why do we need this? Can we narrow the scope at all? 
good catch. this is correct. 
Also here it would be better to use Crawler.Attributes.CATEGORIES instead of the string value "categories"
Should be `publishStateResetEvent`, or?
Should we use full path rather than a wildcard?
Personally I don't like usage and declaration/initialization so far apart. 
Don't create types directly. Use a typeFactory. Or in this case, use typeFactory.builder(). 
The API already assigns a uuid to an OpenmrsObject on construction 
Can we make this constant configurable?
why -1? 
i wouldn't use concept ids and datetime to find this obs , if the list already had some obs by the same concept and you want to get the newly created obs, a cleaner way is to find the newly included obs in the list, the commons-lang library CollectionUtils.subtract(coll1, coll2) method that which when given two collections it gives you the items in one collection that are not in the other which in this case is 1 and that should be the newly created obs, don't you think it is neater? 
newline 
Also here it would be better to use Crawler.Attributes.CATEGORIES instead of the string value "categories"
This matches the existing behavior in lines 443, 460, 530, etc. where mDisposed is not checked before running the callback.  It prevents flagEndAsync() from blocking the callback.  
'one-hot'? 
typo in 'htpp'
Compare the target's location to the player's eye location. Right now, the code is comparing the player's location to their eye location.
Use `currentHigh` instead for clarity. 
Any specific reason to go with `LinkedList` here? It's not wrong, I'm just curious. 
setCancelReasonComponent to match kickReasonComponent?
More messed up imports. 
What is the `file` for? Is it necessary?
It is not right, the minimum value is 5 
![Codacy](https://www.codacy.com/assets/images/favicon.png) Issue found: [The String literal "              (name != null ? \"name=\" + name : null), " appears 4 times in this file; the first occurrence is on line 186](https://www.codacy.com/app/Chris-Purcell-39/FreeBuilder/file/2277828626/issues/source?bid=3146243&fileBranchId=3187161#l186) 
introduced a ton of whitespace changes, plz fix.  tabs only.
Tests provide line coverage for this `if`'s body but they don't provide mutation coverage as it is impossible (hard) to detect logging statements. Should we throw an exception instead?
Why not in the original package? 
When you say leaf, I was assume one of the  obs should not be an immediate child but i see you're adding both as children of the same parent, i would expect a child-parent-grandparent tree 
No need to change. I feel that it's from the refactoring action
`Level.DEBUG`/`Level.INFO` perhaps?
I know that you like verbose, and i could understand that it can be more meaningful. But as you know, i like more CBR or VBR, it's more log readable to be. Too much text / verbose kill the informations; it was just a share of my point of view only, so i'll not bargain for a change.
Same nit 
Its not clear to me that this is correct. Wouldn't you want to check name and type? 
Please take a look at the mappings and annotations here... 
This is not right because the provider who prescribed the order is not always going to be the same person to discontinue it, so don't copy this field 
Can I ask why you use `Map.Entry<String, Object>` rather than `KV<String, Object>` for instance? 
can you restore this line? (annotation and member on the same line)
Please do not use an if-then-else to make the differentiation between 5 and 7 stars. Instead create a new method in the connector interface, and implement it to return 7 for your connector, and 5 for the AbstractConnector, where every other connector is derived from. 
~~hm hm... technically File.pathSeparator would be better?~~ 
may be write that you create temp folder? 
just noticed this one, @QueryParam is still `deployId` here instead of `host` 
![Codacy](https://www.codacy.com/assets/images/favicon.png) Issue found: ['{' at column 5 should be on the previous line.](https://www.codacy.com/app/cgeo/cgeo/pullRequest?prid=349169)
When does it throw a CatalogException? 
I'm not sure we need this
I suspect not every tag that supports NfcB will be CPES, so more might need to be done here. 
> any way, {@code 0} (Normal) is returned.
Clears up some generics warnings. 
nit style: If it can fit on one line, good. If it can fit on two lines, all params on second line. If it can't fit on two lines, each param on a new line.  ``` java public ParseQueryRecyclerViewAdapter(     Context context, Class<? extends ParseObject> clazz, int itemViewResource) { ``` 
remove this check, essentials already does it 
I want to clarify one point. This is not necessary in terms of the servlet theory and it is only a protection for developers (means that theoretically you can change the content type after commit but it's a non sense). Is it right what I say? 
@Koizumi85 I've changed this to ALWAYS create spans. That's a change in comparison to your impl (you were creating spans only if tracing was there).
Change `isHasNextPage()` to `hasNextPage()` 
false /\* validate */ 
The upgrade recipe will need to make note of this (as [MINOR]), as some users might have a custom Ranking implementation.  
please indent parameters 4 spaces, not 8 
`failOnLimits` should not be checked for this quota
As per the above, we should see if this can be re-used across all downloads and thus should be an instance variable.
You test end > max above but do not check for (start != null && start < min). Is this be done by intention? 
Rest of optaplanner code does the () immediately after the var itself, so: "... notFinishedWarmUpList (" + notFinishedWarmUpList + ") is not empty." 
Ditto.. call this newCallbackBuilder() to support static imports. 
This description seems vague, but consumed parallelism is well-defined and unambiguous - it's the number of split points returned so far - 1 (or alternatively, number of processed "blocks" - ranges of records between two split points). I think it's best to refer here to RangeTracker for definition of split points, give this precise definition, and make the [ul] below be simply an illustration of how to apply this definition. 
I doubt anyone will actually use equality. PlanningId should be the primary way to test.
Reducing scope can affect users and blocks adding other implementations.
why does the tests care about WifiManager? If you need to prevent download from server you can turn off download itself from DataDownload? Just need to set a flag right? 
:+1: javadoc :+1:  
This kind of imply that these 4 conditions are mutually exclusive, but they are not. For example, we can have a case where a broker doesn't have any partition and thus satisfied the last 3 conditions.  And alternative is to have the following logic in run():  ``` if (isPartitionLow) increasePartitionNum() if (isAnyBrokerNotPreferredLeader()) reassignReplicas() if (isAnyBrokerNotElectedLeader()) doPreferredLeaderElection() ``` 
Functions that are called in the constructor should be use the final keyword. Without the final keyword a derived class could overwrite it and could stuff before the parent class is fully initialized. 
The `MySqlTaskContext` has a `gtidSourceFilter()` method that returns a `Predicate<String>` that you could use. If we want a config option that specifies whether the events should be filtered with the GTID sources, then this could check that and if true just use the result of `gtidSourceFilter()`.
Can we change it to interface? If we can do that we don't have to make each variables final. 
can you reassure me on why we don't need these client.close() anymore? Thanks
Consider putting this code into ParseIssue. 
A method that returns `null` always ? Why are you implementing it here ? Maybe `AtomBase` could be abstract and that way you would not need this method. Unless you really need `clone()` to return `null` for something.  If you never initialise AtomBase then it should be abstract. 
I've moved the 2 parameters for the cgeo marking and the session ID into the apiRequest method. As the session ID was already there, it is no longer duplicated and managed at a central place, so no other search method needs to handle this anymore. 
replace with deleteFile() util 
I am surprised checkstyle isn't catching this, but things are really compressed here w/ no spaces
this works, although IMO it's kinda oblique from the code; `values().iterator()` actually iterates in the map itself, so the `remove()` call removes the node safely.  > https://docs.oracle.com/javase/7/docs/api/java/util/HashMap.html#values() 
~~this return should use "&&" not "||"~~ My fault, your implementation is correct. 
删除
I fixed this, so tests should work if they are launched from eclipse
![Codacy](https://www.codacy.com/assets/images/favicon.png) Issue found: ['private' modifier out of order with the JLS suggestions.](https://www.codacy.com/app/cgeo/cgeo/pullRequest?prid=348284)
I suspect the message should have been "Lambda creation failed for setterMethod", am I correct?
In which case can this be true? 
remove this, orthogonal. 
Could you use the `org.deeplearning4j.examples` namespace please ?
what is the difference between `SynchronizedMiniList` and `Collections.synchronizedList()`?  
this comment was meant for the next method and next method comment should go here 
Use hashset? 
Is there a point to this method? Would a user ever try to create an injector without at last one module to start with? 
The formatting does not look right here. Did you run mvn clean install before committing?
Since we take care of a `null` entry, I'd use `@Nullable` here as well for `text`.
I copied it from the zookeeper code. It doesn't appear to throw  `InterruptedException` either, but maybe I'm reading it wrong. I see what you mean. 
Revert these changes 
default is true 
I thought I had let a comment on this line but I can''t find it.  You could maybe use something like (untested):  ``` java    database.update(dbTableWaypoints, values, "_id = ?", new Object[] { oneWaypoint.getId() }); ```  which is safe in any case (here it is safe, but discouraged, to build the statement yourself). 
Unless we have a specific optimization for this we can take the exclusion criteria as a `Predicate<ClusterNode> exclude` to make it less specific?  The call would (according to my napkin coding) then be: ```java withNodeInShardButNotWithId(shard, excludeIds::contains, fn) ```
Returning from here was breaking too many test cases.  So left it as it was. 
Nd4j.getAffinityManager().getNumberOfDevices()
The formatting here, and in a number of other places, does not look correct. Did you run mvn clean install before committing?
@saketkumar95 remove all extra blank spaces!
I wonder using getContext causes memory leaks cuz it just returns the Context this fragment is currently associated with. Should we use application context?  refs. https://github.com/rejasupotaro/kvs-schema/blob/06d9eaf3b7b156d7e86b6115b0d2bb68129af77c/library/src/main/java/com/rejasupotaro/android/kvs/PrefsSchema.java#L12 
Blank line
Because explorer also can open URLs. e.g. explorer.exe "www.jabref.org"  
Same here. No need for reflection... 
I'd probably argue we should not try to animate the fill for this animation, since the path is unclosed until the very end of it. If anything, I'd say we wait until the stroke is done animating and then fade in the fill color.
On any event builder? that sounds like a type mess. Is this the "not perfect" you were talking about? 
please add: error.put("accepted", false); to make processing in client easier
This one is missing from `master` branch too ! 
As a result of fixing the keybindind stuff, there now needs to be a plus sign between the modifiers and the key, otherwise it may not be recognized correctly. Otherwise the code looks good, thanks for your contribution
improve naming please 
"to where" (strike "the position")
this is meant to close all possible activity scopes inside ondestroy of each activity?  Probably we should add javadocs
How does this implement splitting? I don't see any difference between the sources -- how do they divide the events between them? 
it's the same as setEncounterProviderId 
missing license header which is required nowadays :) 
Make indentation in this file conform to the rest of the project (2 spaces vs 4) 
Sloppy copy/paste job? 
you should add a comment to say this method is not synchronized. 
Is this a general purpose method to check for duplicate names, if yes, then it should only check for duplicates in the passed in locale and not the broader one 
You will need to compare class objects i.s order.getDosingTypeAsClass().equals(SimpleDosingInstructions.class) 
Seems like a semantic change for users who weren't setting this -- why is this OK? 
Is this intentional, allowing `...` to end the block but not to start it? 
Did you mean HOUR based instead of TIME ?
I think it will be better if you convert it to several if's...
`@EqualsAndHashCode`?
variable renaming: filter -> transformer 
Guava has MediaType class. It may be useful 
This method name must match your @should annotation.  "@should delete item from database".  So either make the annotation "@should purge ActiveListItem" and the method.  See the eclipse plugin to have eclipse generate your method stub for you from the @should anntoation: https://wiki.openmrs.org/display/docs/Unit+Testing+With+at-should+Annotation 
Is it worth testing `matches("GET/com", vars -> assertEquals("", vars.get("rest")))`?  Notice that it doesn't has the last `/` in one of your test, it seems to me another corner case but not 100% sure. 
nit: we can use `never()` instead of `times(0)`
Please use `#0`, `#1` as variable substitution 
nitpick: can you remove this whitespace?
we are using 4 spaces indent
I wonder if there's also a `subtitle-loc-key` and `subtitle-loc-args` field that hasn't been documented. I'll try to test that a bit later today, too. 
s/public static/private static/g
I though the ticket changes would not require modifications in this test. Not so?
aren't some of these tests already there? 
maybe extract Thread.currentThread().getName()? then you can use a format string (and avoid string cat when debug isn't on) 
I personally am not a fan of this way of logging(I've just  sent an email), but I guess it's best to be consistent :-) 
Currently you can provide them only with a property when running, any variable value defined in `<build>` is used as a _default value_. I think adding it always a default values make sense.  I wonder though whether it might make sense to add an extra configuration section (like `<argValues>` or so) with values to fill in for overwriting these defaults so that different Maven profiles could use different values for this. But thats probably to far fetched, so better stick with the current soluton (just thinking loud ;-). Since this would be an addition it can be always later added later (adding is easy, changing hard) 
What. 
Please use `download` instead of `dl`, or just omit it. 
When we are using this extension, then we should also store the personal note as userData. But that should be handled by another issue, not this one. 
![Codacy](https://www.codacy.com/assets/images/favicon.png) Issue found: [Avoid long parameter lists.](https://www.codacy.com/app/cgeo/cgeo/file/4460069094/issues/source?bid=3217125&fileBranchId=3916848#l45) 
这个变量应该也不需要了 
SimpleDateFormat is not a thread safe class, so i worry when i see it in a static variable.  i think the solution here is to inline this into `formatDate` and just pay the cost of creating one every time its called. this isn't perf sensitive part of the code so its fine. 
minor: typo
This does not support non-repeatable `Payload`, e.g., `InputStream`. 
I think Files.find or Files.walk can be used here as well, because a path can also be a directory 
not really needed - remove
In that case all the test without real class name will be in the same group in `xunit` tab, isn't?
As we don't modify the map after creation, how about we make this a regular immutable Map?
Use `android.R.string.ok` 
Use `getRawPlayers()` to skip the cast. 
Same as above, needs the lock on "conferences".
why do we need this class? can't we store everything in the JpaCacheStoreConfig? 
typo: ProducerRecord (not ProduceRecord) 
Change this test's base class to BaseContextSensitiveTest
can you add braces?
I think that's going to be a challenge. I'd prefer to have an imperfect implementation of the timeout than nothing though. 
What is the effect of this change in old, 5.1.x, clients? 
There is a risk in this being separate from cooldown secs. For example, the VM maybe enabled too soon and start getting assignments before it is terminated.
Style nit: can you put the body on its own line? 
I do not understand the purpose of all of these `if` statements. 
Please use 4 spaces as indentation
I suggest to get rid of "done" here too, as well as everywhere else. 
Please delegate to `addMembership(CohortMembership)` 
test prefix to method name would be nice to have 
Minor: sp - `HANLDER` -> `HANDLER` 
Could this be other than ```Aggregate```? It seems to me from L101 in this same class that the OrderBy/Limit is applied after the PostAggregation Project. If this can be any other operator, which ones? Otherwise, the ```if``` clause should go away.
Prefer Guava `BaseEncoding.base16`.
This test would also throw `DateTimeException` without the fix. Now it "succeeds" with IllegalArgException explaining that Decades increment doesn't fit into the `[from, to)` by integer number of times.
The type could be built once in the constructor 
NotNull annotations missing
Should we use `ImmutableList` for these? Also, where does the list come from? It makes total sense, but should we update it when new releases of OpenSSL are made? Or when vulnerabilities are discovered? Does OpenSSL have a "good" list we can reference rather than copy? 
Would prefer calling this say `DefaultMetricsReportingService` or similar 
Maybe I'm wrong about the best practices but if I were implementing this I would be passing this to the LifelineDelayedEffect constructor rather then using the value system. I guess that's something of a stylistic choice though. Also, does this work if there are multiple triggers in one turn? 
Looks like the file names aren't changed, this file is still named `YAMLFrontMatterBlock`. That's why the Travis build fails. Are you on a case-insensitive file system? Check out this [Stackoverflow question](http://stackoverflow.com/questions/17683458/how-do-i-commit-case-sensitive-only-filename-changes-in-git) for how to work around that. 
Checkstyle: Parameter enable should be final.
how is this method different than the base method from OAuth2AuthProvider? We should not duplicate this method, only extend it. 
Why set bean to null in the one line and then using new ItemHistoryBean in the next line? 
Test would likely be easier to debug if it was separated into different cases -- eg., simplest case, dropping case (just verifying that data was dropped), triggering case, etc. 
Comments should have a space after the `//`
I think you can just make this a 'byte i' and you don't need to cast. Probably doesn't matter. 
Remove comments 
This should be able to remove a membership that matches the patient, if there is multiple for the same patient then you can ignore it 
Nice, but AFAIK MediaInfo is not able to parse it...
Please use 'final' when possible
still need something here it seems ? 
Is this commented code needed ? 
This should be isShowSplashScreen 
TODO + DEBUG
Should we make it Map<T,T>  ? I think it is better idea.  
getRowCount() may return a different number of rows than the actual bootstrap is seeing. Consider the following scenario: 1. 1 million records in table. 2. getRowCount() returns 1 million 3. insertBootstrapStartRow() 4. 1 million rows deleted by outside process 5. Maxwell sees the bootstrap start row. 6. Maxwell does a "select *" and inserts 0 rows into the stream. 7. Maxwell marks the bootstrap row as completed.  insertedRowsCount will always be 0. This while loop will never set isComplete and will block forever. 
why the function name change
Update the comment.
Can you remove this import?
when we use the new signature, we can remove all response operations but leave the logging.  ``` java protected void deliverResponse(Response response) {    response.log(); } ``` 
I'd rather use ObjectUtils.nullSafeHashCode(..) here and above. 
I think this is a bit misleading, I would rephrase it: `(Only if 'struts.devMode' is enabled or a 'disabled' attribute is set 'false')`
`visitedHashes` is never used in the function. better just pass straight to `getLatestDiff`. otherwise, it's confusing: has a name -> has a purpose.
why do you mix slf4j and log4j classes and properties? 
As I suggested earlier, you are calling setFlashIcon on all if and else conditions, you should put this out of the condition stmt.
This is only read or written from synchronized blocks - so `volatile` isn't really needed. Inside synchronized blocks you'll never see stalled data.
Why `Xor<Void, T>` over just `Optional<T>`? Or if you're worried about the loaded/not loaded yet case seperately, `Future<Optional<T>>` ?
Can we leave out refactoring like this to minimize the diff to be reviewed. 
the same as in `isPresent` case 
We recommend using [`Create#withCoder`](https://github.com/GoogleCloudPlatform/DataflowJavaSDK/blob/master/sdk/src/main/java/com/google/cloud/dataflow/sdk/transforms/Create.java#L70) instead of setting the coder on the output PCollection.  The reasoning is a little subtle -- any PTransform that changes the type of its input at runtime needs a strong handle on the TypeDescriptor (or at least the Coder) that a user may want to use. This is because the runner-specific implementation of `Create` may be a composite PTransform that needs the coder in `apply`. Note that the output PCollection is created during apply time, so naturally the output coder could not be set yet. 
The `assertTrue` should not be necessary: the `expected` in Test makes the test pass _if and only if_ the exception is thrown. 
Would changing the value (expression) entail building and wiring up a new ConstantBox, or would `expr` be mutable? Having it mutable would perhaps help SliderBlock, but I'm not sure of the tradeoffs. 
Is there a way to get back to basicFactory mode, I mean well written code shouldn't need it, but still. 
Do we consider a newly added replica's bytes as `dataToMove`? Should we clarify this with a comment line?
Shared Fate works for all players this text is incorrect.
Backwards incompatible change for tests. 
Isn't this redundant`? If the path is giving an Optional.Empty and you check with ifPresent then you already checked if the file/path exists.  
Should these be in upper case since these are constants? Do contrast with the token binding spec as well.
Use == here. Method name makes usage clear
`org.apache.kafka.common.config.ConfigValue` is not `Serializable`, so if the intent of this exception is to be `Serializable`, I can see a problem with holding on to this `Map`  Perhaps converting the `Map` values to something else or simply making it `transient`, depending on what the intent it is a better alternative
This is a Cohort validator and not membership 
Can you make this explicitly use the `FluentWait` default?
Unused imports: import java.nio.ByteBuffer; import java.util.Collections; import java.util.HashSet; import static java.nio.ByteBuffer.wrap; 
The subject should be topic-key or topic-value. 
This method `TestingUtil.cachestores` is undefined 
Nit: Should go after tests for start. 
To return 0 or NaN - I would lean towards NaN. 
In retrospective I would say this API is not very OSGi like. The context parameter should be removed. A service factory should be used. The requester should be able to request a special SSLContext by providing a context string via a service parameter.
Galder, doesn't this invalidate ALL the connections to that server? I think it should only invalidate the current TcpTransport.  I think transportFactory.invalidateTransport would be more appropriate when catching a RemoteNodeSuspectException, WDYT? 
nit: final
I think you could use the MockitoAnnotations to create the mock runner as well, and do more initialization in setup. 
You can leave out these TODOs. Normally it is nice to return a useful message like "Finished updating Encounter Roles" since liquibase does something similar for every changeset 
We can use getContext right? 
Add jar name to exception to help debug errors 
same here, the type boundary is no longer needed 
Whitespace
I do not understand why are you doing such replacement? Java has mechanism to recognize what OS is running on and what is the path separator. 
Please remove the empty lines between the variable declarations or better group them (e.g. all GUI/panel related stuff first) then a new line and next group. 
Map 缓存
The name `fooRecordsOfType` needs work. It's alright for `objectRecordsOfType`, but will end up sounding like actions for the collection types: `setRecordsOfType`, `listRecordsOfType`, and `mapRecordsOfType`.  Would this be better:  * `objectsOfType(typeName)` * `setsOfType(typeName)` * `listsOfType(typeName)` * `mapsOfType(typeName)`
Would be good to be more clear that this extracts the `after` field, either in the docs or in the class name. 
You need to use an additional LockedInDynamicValue, otherwise the boost value will change as the number of attacking creatures changes during the rest of the turn. 
For API (Glowkit) Support call super methods here? 
移到baseAdaptor
what's up with those one line methods and multi-line comments? smt related to tests? 
s/tabs/2 spaces/g 
This is incorrect, the unvoid handler should unvoid any memberships of the patient whose endDate matches the patient's dateVoided 
We could potentially have this inside an if (INFO) statement. 
query -> queries 
typo: unkown 
When will this happen? Can we test it locally realistically? Will it ever happen?  Don't continue, don't log an error, just fail fast and throw an exception. When in doubt, leave it out: when in doubt, don't support the functionality, break and fix it when users starts complaining about it. This way we don't have to support misbehavior for ever due to backwards compatibility. 
`PartialUpdate` changes its own state and creates a new instance. 
probably better to do name = name.toLowerCase() here? 
Can we remove the unused `numTotalPartitionMovements` method from this class?
voidExistingObs(), This method seems to be just saving rather than voiding an Obs and you've already marked the Obs as voided, unless you intended to move setting of voided fields to that method.   
does this mean it will attempt once, or retry once? it might make more sense to rename this to something like `maxDockerPullAttempts` 
I don't like this catch. All this will do is cause NullPointerExceptions down the line, so why catch it at all? 
I'm not in love with this name - it overloads with `ItemSelector` (not a big problem), and with the yet-to-be-introduced `CandidateSelector` that will abstract the notion of identifying a default set of candidates for a recommend operation.  What if we call it `GreedyRerankStrategy`, so that the name indicates clearly the use of the Strategy pattern?
I think you need to get rid of the braces because i doubt if the test generator will like it, did it generate the test methods? 
Considering that these are internal interfaces, does it really make sense to create a new `Optional` just to make the code more functional? Even with `find()`, I've seen quite a few `ev.find().orElse(null)` in your PR, so the benefit isn't very clear to me. 
This will loop forever if the user sets a negative size.
is this key unique enough if there are multiple fields with the same type, either in the same class or in class parents? 
This group ID seems to be replacing some setup we previously did in Kafka store, but this doesn't match the values it previously did for the schema registry service. This would be a compatibility issue since that ID is used to uniquely identify the instance. See comment later on for more details. 
No output in tests, if we can, they are just debug strings, if we need prefer logging with trace levels
![Codacy](https://www.codacy.com/assets/images/favicon.png) Issue found: [Avoid unused imports such as 'java.net.MalformedURLException'](https://www.codacy.com/app/triplez/open-event-android/file/2406715500/issues/source?bid=3233850&fileBranchId=3233904#l13) 
change this to: JSONObject cookie = getPayload().getJSONObject("cookie");  then change the references below to just cookie 
A new server will never have it as 1 right? 
If this is intentially not a PlanningEntity, can we change that line to `// Intentionally no @PlanningEntity annotation` instead?  Diagonally scanning this looks like something that was temporarly uncommented and forgetten to be commented it.
This could be a singleton 
I'm not sure how I feel about splitting the Catalog in two - it seems you'd have to duplicate quite some XML-related nonsense in doing so. A possible alternative would be to have one catalog that knows about both functions and typeclasses, a `catalog.xml` if you will:  ``` <catalog>   <typeclasses>     <!-- as here -->   </typeclasses>   <functions>     <!-- as before -->   </functions> </catalog> ```  As an additional bonus, this would perhaps make it easier to detect if we're getting bogus typeclasses in our function type definitions. 
Log the exception 
![Codacy](https://www.codacy.com/assets/images/favicon.png) Issue found: [Avoid declaring a variable if it is unreferenced before a possible exit point.](https://www.codacy.com/app/cgeo/cgeo/pullRequest?prid=537896)
opposite here 
I don't think the macro should be called `srd` as that would seem to imply the REST Docs will only ever have a single block macro. There doesn't seem to be a tradition of trying to namespace macro names in Asciidoctor so I'm tempted to go with `operation` instead.
the package names seem to have gotten a little wonky. I cannot tell exactly when.  This should be `com.google.cloud.dataflow.contrib.firebase`, and the same throughout. (See, e.g., https://github.com/GoogleCloudPlatform/DataflowJavaSDK/blob/master/contrib/hadoop/src/main/java/com/google/cloud/dataflow/contrib/hadoop/HadoopFileSource.java#L17)  [Feel free to use subpackages of c.g.c.d.contrib.firebase within the module as you see fit, of course] 
![Codacy](https://www.codacy.com/assets/images/favicon.png) Issue found: [Parameter targetFile should be final.](https://www.codacy.com/app/cgeo/cgeo/pullRequest?prid=531654)See long comment above in onDestroy. 
Please remove this annotation. 
Since these are public fields, don't name them with the `m` prefix. 
While you're fixing the indentation, mind maybe adding a blank line above here? This looks rather jumbled as-is. Everything is just squished together. 
If this test is isolated, why should there be any tables to drop? 
Please move the reading of all configuration properties to the initialization function. 
How about having an optional flag which enables or disables the "simple mode" which you are using in your app? I would guess that most apps that would use this ContentProvider would probably want to get the full Q&A including the CSS, rather than `qSimple()` and `getPureAnswer()` 
Is this comment valid given that we have paramIndex ? 
Have you tried using the GeoTools Converters instead? Converters.convert(value, clz) 
This `if`-`else`-block is still logically wrong. Check [my previous comment](https://github.com/HeinrichReimer/material-intro/pull/155#r96389088)!
This method feels speculative.  Since the implicit contract here is that we'll supply all the methods in this interface for you, I think it's ok to leave this out and then add it later only when it's actually needed.  At that point, `getHandle()` could be a default method that simply calls `getHandleSupplier().getHandle()`.
While this sounds pretty good, consider a possible condition. Suppose a user consistently uses an overloaded method. Every one of those queries will need to check with ZK. Drill is supposed to handle many concurrent queries. Each of those will trigger the update. Soon, we'll be pounding on ZK hundreds of times per second.  The "not found" case was fine to force a sync since a user would not, presumably, continually issue such queries if the function really were undefined. But, the overloaded function case is possible, and can lead to performance issues.
From now on, lets standardize on the following logging format which avoids string concatenation overhead if logging is disabled: LOG.info("timestamp pattern: {}", patStr); 
Transforming the `Chain` into a `List<Operation>` before processing the list looks fairly expensive. Ideally, the `resolve` method should work directly with the `Chain`. 
minor: info is always enabled. The test just obscures the code. Even most of the LOG.debug() statements don't need such a check. 
Could we remove this enum and add some static factory methods to this interface? 
"to function"? 
how could this test pass if a stat observer was missing ? 
Incorrect formatting - `finally` has to be on the same line as the closing brace of the `try` 
nit: line break (bad Android Studio) 
I would prefer that the default is RAISE_ERROR ( CACHE_CONFIG_IN_MEMORY can easily become a memory leak) 
This needs further explanation...  It doesn't affect queries or?
i think this comment should go elsewhere and not b/w the `if` and `else if`
There is a lot going on in this method. It could easily be split into multiple to make it readable.
I'm trying to reduce the number of unique `RestServiceErrorCode` - otherwise we can come up with a code for every new situation. 
class A < B > is covered partly by this 'if' (and probably that's not what you expected to be fixed by this code). At first I was afraid that type2 == GT check is dangerous for relational operator. It's kind of luck that it is not affected. I think it would be good to make code easier to understand by adding one more check in addition to type2 == GT, for example type2 == GT && elementType == SOMETHING. 
Misformatted code 
Again, why?
// for each page in pages do for (Page p : pages) {    p.authority = 1;    p.hub = 1; }  More closely maps to the pseudocode. 
Can `context` ever be null? I think this check is unneeded.  
myRootsHandler.getTrackedProjects() will be better 
This will not work in runtime, mixin classes aren't available. 
Why not string? I see alone enum.  
No memcache? 
[Optional] Since all you're doing with `addresses` is iterating it, consider declaring it as `Iterable<InetAddress>`.  
You can leave off the "Internal method" part in the doc for these methods.  If it's public, no one cares if it's intended to be internal or not ;)  If you really want it to be internal...don't make it public. 
Added this extra log when using shaded Netty. 
final
Same as above 
new lines... :) 
spacing
oops. should be "value" there. already fixed 
I think that for consistency we can safely use `jacoco` folder here - module with aggregated report is not supposed to have other jacoco report. 
URI should not be generated in toString() method, there should be a separate method  for this. toString should be used only for debugging feature preferrably 
If an error should never happen then we probably need to know about it: Please throw a ````RuntimeException```` or an ````Error```` instead of discarding the ````ExecutionException````. I'm not sure what the best behaviour is for handling  ````InterruptedException```` though.
No need to specify default value if #29 is merged. 
Most columns have a constructor like:      public DoubleColumn(String name, double[] arr) {  I see `BooleanColumn` does not. It'd probably be good for you to add one so that creating these columns would be simpler like:      BooleanColumn booleanColumn = new BooleanColumn("col1", new boolean[]{ true });
Is this comment useful?
Odometer should be in meters.
The return type should be Collection<URI> since order is not important. As mentioned in my previous comment, the type for tag should be URI too. 
Why easymock? Aren't we usually using mockito? 
Should be `getUniqueID` as per guidelines 
Needs to be `>=` :P  
Duplicating `newContact` in every test makes the reader compare every test setup to see if there's a difference, and if there is (for whatever reason), is it for a good reason or merely an accident? It seem they could use the same basic contact as a starting point. 
So is this 16 or 20? I say 20 if vanilla supports it correctly. 
The CSS selectors BODY and .card are redundant given that you have '*' 
I found this really confusing. I know it works (TBH I wrote a test to verify it), but it's going to give pause to anyone reading the code. For the sake of conventions, default clause should go last. 
AFAIK `is*` can be used only with primitives, after making it Object it should be `get*`( or `has*`) 
This bit `positions != null` is unnecessary. 
Where does this class come from? I don't see the dependency in the `pom.xml` file. 
Shouldn't this only necessary if `branches > 1` 
and here. 
I would suggest implementing the `run` like the following so this and the method below are guaranteed to be called. The actual logic of verification should be implemented by the subclasses in the method `verify`.  ``` public void run() {   afterVerificationStarts();   boolean dataComplete = verify();   afterVerificationFinishes(dataComplete);     }  protected abstract boolean verify(); ``` 
Not directly related with this PR, but it seems to me that package hierarchy is getting too deep. Let me send a follow-up PR that simplifies this.
Revert 
let's rename `FilesException` to `RuntimeIOException`, URL are not only about file. 
``` Caused by: java.lang.UnsupportedOperationException 	at java.util.Collections$UnmodifiableMap.put(Collections.java:1457) ~[?:1.8.0_91] 	at com.sk89q.worldedit.world.registry.BundledBlockData.loadFromResource(BundledBlockData.java:137) ~[main/:?] ```  Did you test this code or just assume it would work?  same for below.
I'm becoming increasingly uncomfortable using these `@Value` Strings. They're hard to hunt down when we want to change something and when we give them a default value like this it doesn't really break anything if we say change the name elsewhere this will still work. It might be better to create a `@ConfigurationProperties` class that at least binds at compile time. Also I would add whatever default value you have to the properties file so we have a record of it when we go to document.
answer is unused 
@NielsCharlier try-with-resources? 
s/dockerDirectory/directory/ 
Shouldn't this use the new `IBillingPastHistoryHandler.onPastHistoryError()`?
Any reason that it's not zero-based?
How about renaming to `ServerGroupRegistry`? 
Wow this is subtle. I think @pnarayanan is right. The assumption might not always be valid. 
at all could be written without else :) 
What does `httpConnManager.toString()` return? 
`return ~potentialIndex`?
This needs to be addressed, there is no comment describing what the class does 
You can just use `SyncthingService.class.getName()` instead of writing down the ful class name.  Also, I didn't even know you could add a message to the assert like that. Prett neat! 
Should check if the return value of listStatus is null. 
Is CouchbaseLiteException thrown from anything in this method?  Or it should return void but throw CouchbaseLiteException instead?
Yes, they are. Maybe the comment could explain that this exception is thrown most of the time and one needs to use the mirrors to inspect the class. 
I wonder whether it would make sense to move the lookup into a static final HashMap<String, Integer>, so the binding code would become:  ``` int eventIconAttr = EVENT_ICONS.get(event.getEventType()); if (eventIconAttr != 0) {     holder.ivIssueEvent.setImageResource(UiUtils.resolveDrawable(mContext, eventIconAttr));     holder.ivIssueEvent.setVisibility(View.VISIBLE); } else {     holder.ivIssueEvent.setVisibility(View.GONE); } ```
This interface has been changed to use the new class - ReadableStreamChannel 
That's a good point. The alt option would be to pass in the `ResourcePool` 
Looks like you forgot to re-use `sleepVal` here.
Indentation of comments. 
HashMap -> Map?
Sorry for being capricious, but I realized we extend `CompletableFuture` and thought we may want to mimic the parent API (e.g. renaming `successful()` to `completedFuture`) or just forget about using static factory methods.. What do you think? 
What does this command do? It doesn't sound generic enough.
It can't fail because we use regular expression, so I don't think we need to catch anything. We can just use `parser.nextInt()` and pass int value directly into `decodeAlarm` method.
"Chunk" 
Lovely! 
Since the mapping registry automatically adapts column mappers into row mappers, you could just skip the column mapper part. 
Yes, that seems good.  To avoid GC problems, the generated class can use clinit to create/store an instance of itself in a static final, and then Companion can hold a WeakReference to that instance.  Not sure if it's worth avoiding even a single getField, but if so, you could add a method to return the instance. 
This could be made slightly more efficient if we do (sourceIsInCurrent && destMatchesClientFilter) check and if true we could skip the calculation for destIsInCurrent and sourceMatchesClientFilter.  Similarly with AccumuloIDWithinSetRetriever. 
- Could we have a version that accepts `Iterable<? extends T> labels`? - How about renaming to `metricLabels` for consistency with `PrometheusMetricRequestDecorator`?
This seems really broken... am I missing something here?
Assuming you're using the java language default of 0 for integer here rather than saying it explicitly
These are BAD_REQUEST responses. We no longer close the channel when there are bad requests and the method is not POST.  Also, this check never logically belonged here since `NettyMessageProcessor` is not the one who handles keep-alive or closes channels. Those tests are now where they belong in `NettyResponseChannelTest`. 
for clarify I would add `CustomParseException` with `line` and `column` as public final fields. 
Key class is not static so there is no need to have duplicated `Equal` / `Hash` field 
So there is a TestHarnessExecutor and a TestHarnessLauncher. Both have a launchTest method. Could you shed some light on the roles of each of these classes? 
typo : `recursivly` 
Shouldn't this still have the `*` on the left? 
This new interface somewhat overlaps with `com.datastax.driver.core.Clock` and thus makes the improvements brought by JAVA-727 unusable within this new policy. 
Whoa, i just deleted your comment on this line (i meant to delete mine). You were saying how i shouldn't null/empty check myself. As per side-conversation. It would be nice to test these annotations work, but it's out of scope for this. Removed null/empty checking from this.
I'm unsure if we're going to be exposing this. 
Can possibly collapse the two try
Maybe it'd be useful to have a log message here saying that it was successfully stopped 
small typo here "node" should read "none"  also this doesn't actually check if both of them are not null, let the message seem to suggest 
Can you put it on the other line?
The [`Deque` API](http://docs.oracle.com/javase/7/docs/api/java/util/Deque.html#addFirst%28E%29) recommends using `offerFirst()` instead of `addFirst`. I think it would be simpler (and perhaps more performant) to check the return value of `offerFirst` than to catch this exception. 
The fix itself was just this `&& !mTiActivity.isActivityChangingConfigurations()` line. 
To improve readability, we could call this `Server` "serverFromSystemProperties" and `s2` could be called "serverFromDirectParameters", or something similar. 
How about `com.linecorp.armeria.client.http.retrofit2`? I'm not sure which is better to be honest. What do you think? 
@oberstet One option would be to pass in the serializer to onMessage() handler method, giving the Session access to the serializer.
The priorities should be reversed: get the creds from the properties first and fall back to the env. 
Curious why you wrap those exceptions into `RuntimeException`, why not just throw the original exceptions?  
This is scary when I realized it was passing a mutable map to a totally different thread.  I guess the reason it never became a real issue is because for user MP procs, irrelevant fragment responses from before the restart are dropped on the floor, and the borrow task aggregation accepts empty dependency by accident. However, sysprocs don't tolerate empty dependencies. 
doc 
It would be more helpful here if you describe `ByteBufferPool` itself at a high level here (maybe) in addition to some basic functionality. Maybe the line about exception is not needed here but instead at `allocate`? 
not directly related to this commit, but you should use KeychainService.this in these places instead of a field. or do you actually need this for anything? 
In stead of creating a DumbLogicalExpression augmented with type, it's better to do the opposite way.  Slightly modify the original one such that it accepts a list of MajorTypes. It only requires to know the argument type, to decide the return type.   MajorType getReturnTypeMajorType(final List<MajorType> types) : This one uses most part of original implementation.  MajorType getReturnType(List<LogicalExpression> expressions) : this one will pass in the list of argument types and then call the first one.  
in `maybeCloseNetworkChannel()`.   
again, calling `.get()` alone...
Huh? 
The quotes “ and ” were causing problems with the encoding on my system, so I changed them to "standard" ASCII quotes ... happy to revert if you prefer the originals though.
Doesn't quite like this added dependency to secor.uploader package, I think you can have this property have a default value in secor.common.properties (there are quite a few config property specifying classname in secor.common.properties already) 
Can you move the location of this to just below `resumeTimer()`? 
This code is repeated in geotools as well. Could be factored out to a static method in the new style visitor for example. 
Javadoc...
predicate -> authorizer
I would remove this fallback. 
Will this work? We leave the metric unset, then iterate over them in the following loop. Also, we build the unknownMetrics set, but never use it.  Suggestion: if the metric is not known, just set its value to 0, and log a message to the log file.  This situation occurs only when 1) opening an old profile with metrics that no longer exist, or 2) when adding a metric but not registering it properly.  Note that this code does not handle the case where the metric is not registered (the metric names is null.)  For the external sort, the problem occurred because we have two sets of metric enums (two implementations) but only one registry of names. The fix was to keep the old and new ones in sync.
Does this mean you will not update the topic schema to latest?
I think int would be enough for max depth. We may also want to rename that field since it's not about graph depth anymore. 
Can you explain why using while loop here? I'm not quite sure I understand the logic.
I see that you're making these protected so they are accessible from a subclass, but I'm now realizing maybe they should be `public static` utility functions on the superclass. They don't seem to need any state fields from the class instance itself.
I think another name would be better as Metric represent something else. Maybe WhatTag? 
Doesn't this mean that the whole photo is displayed in the gridview and not just a thumbnail? This could lead to an Out of Memory problem especially with older phones.
looks like you may be able to re-used methods from org.springframework.util.SerializationUtils for this 
This doesn't belong here, at the very least it belongs in the ExoMediaPlayer class
`final DockerClient docker` 
Any chance you convert the enum member names to upper-case? It's just more common and what you'd expect from it. Not sure though whether there is a good way to automate? 
just wanted to note -- is this the only place to cleanup the thread local? 
possibleValues in the case can be static as it will never change. Additionally, if you use:  Collections.unmodifiableSet(new Linked)  it saves you instantiation an unmodifiable set wrapper every time you call getPossibleValues(). Ideaily, this pattern should be followed where applicable in all the domain implementations as it is more efficiet.
nit: waterMarkObj.getValue() 
* Can this tag not be used on multipolygons? To include relations, simply write "elements with..." * I have been thinking, perhaps the app could also ask the user for the sport if it encounters (kinda) deprecated taggings so that if the user answers, it is overwritten with the proper tag. What do you think about that? I.e.    * [team_handball](https://wiki.openstreetmap.org/wiki/Tag%3Asport%3Dteam_handball) is actually handball   * [hockey](https://wiki.openstreetmap.org/wiki/Tag%3Asport%3Dhockey) is ambigous and should be field_hockey or ice_hockey 
It's better to use `T state = ...` so you don't need to add `SuppressWarnings("unchecked")` 
Minor: Since this is a test, doesn't matter too much. But it looks to me that `sslFactory` and `sslSocketFactory` could be static variables? This constructor is called for every test so you are creating similar objects every time. 
`putIfAbsent` returns the entry that was already there, but if nothing was there it returns null so we end up with a null Meter. 
I think if you also check for emptiness here you'll save an iterator allocation (when the collection is empty).
This check looks not clear for me. Use high-res or not this is only for "Raster maps" and only for devices with HDPI screens. So I'm not sure if it is proper check. 
Please no unrelated formatting changes 
I think it would have been better to keep the name `Saml2Client` for backward compatibility on 1.7.x and change it on 1.8.0. 
@Godin Either we use the "strong" tag here or we need to extend the stylesheet. Currently the "b" markup gets lost. 
what happens here if a method is overridden?  seems like you're going to get duplicate `AnnoMember`s in your setters / getters? 
can you explain this change? 
rename dangling to linkToNonExistent 
delete 28-31
`the {@link SessionProtocol} which the invocation has been performed on` ? (/cc @anuraaga)  I guess we don't need to state it's HTTP-only here. Perhaps we should say so in the `MetricCollectingService`? 
On add/remove, if the user is passing in the position of the parent, can't we use that to get the `ParentListItem`? 
When will the key be null?  For tombstone, the payload is null. 
Small typo here. The answer is awesome.  
Does the import happen at time "now"? That's sort of odd, although I dont have a specific better idea. But anyways, you should probably do a direct load without a time, because the FKE might show up as null here if it's past the deletion time, but in this case that's not what you want - you want to check that no FKI ever existed for this ever.
Is -1 same as infinite? Infinite is bad (usually). 
Why such a short line length limit? 
Since this is CompatV2, we could support a "?query=" parameter in the uri, and pass the encoded selArg in there. So for: "mid=? and csum in (?, ?, ...)":  /notes?query=mid%3D%3F%20and%20csum%20in%20(%3F%2C%20%3F)  and then pass in the String[] {mid, csum0, csum1 ... }  EDIT: actually it would make more sense to pass "mid=? and csum in (?, ?, ...)" as the selection and then have some way to identify that this query is coming from API V2, not V1 
This does not need reflection.  ``((Progressable)context).progress()` would do. 
You could name these two static final fields using UNDERSCORED_UPPER_CASE. 
I think it makes sense to make this configurable with defaults 
we will need to call `ShouldBeEqualByComparingOnlyGivenFields` and give it the fields differences. 
use string constant above for ttl
This needs to be volatile or wrapped in `AtomicReference` to be really thread safe. 
With our immutable builders, we promote sharing them. Should we have a method so you can override a cache with a different configuration?
The comment "from a single logical plan" is not quite accurate since this is taking a POP (Physical Operator) plan and generating another POP plan.  
Unintended changes for debugging? 
Is there any benefit to making a distinction between 400 and 500 style error responses?
new line 
And ultimately this was never reachable because the locale is re-applied through the super call above it. 
Does `consume(int)` support skipping across buffer boundary? I guess another method like `skip(int)` is necessary. 
Jody, all these casts should be superfluous, "class DataStoreCache extends CatalogResourceCache<String, DataAccess>", it should already return a DataAccess no? 
This comment is redundant. The declaration already tells us it's a map of params.
could use `containsNull()`
Is it required to use this `Nullable` instead of `javax.annotation.Nullable`?  
No. There is no reason to do that. The channel is purely for testing purposes. If the remote end had to close the channel or something like that, it makes sense to check the state of the channel. We don't need to explicitly close the channel in tests (unless we are testing some behaviour under close). It is not using any resources either. 
![CRITICAL](https://raw.githubusercontent.com/SonarCommunity/sonar-github/master/images/severity-critical.png) Document the parameter(s): content [![rule](https://raw.githubusercontent.com/SonarCommunity/sonar-github/master/images/rule.png)](https://sonar.projects.vaadin.com/coding_rules#rule_key=squid%3AUndocumentedApi) ![MINOR](https://raw.githubusercontent.com/SonarCommunity/sonar-github/master/images/severity-minor.png) "public" is redundant in this context. [![rule](https://raw.githubusercontent.com/SonarCommunity/sonar-github/master/images/rule.png)](https://sonar.projects.vaadin.com/coding_rules#rule_key=squid%3AS2333) 
Could just do  ``` final List<String> preexistingTestImageIds = imagesToShortIds(preexistingTestImages); ```  if you’re ok with the short (12-hex long) id. 
This doesn't need to throw any `Exception` BTW 
If this implementation is integral to the test, just call `GuavaCollectors.immutableSet()` rather than reproducing the whole thing. 
Not necessary for this PR but did we discuss making these into a Catalog? Its been a while so that discussion, if happened, is lost to me.
These deserialise methods are used a lot, so we should minimise the number of objects we instantiate. Although it is more code, it might be better if   ``` java deserialise(final byte[] bytes, final Class<T> clazz) ```  just delegated directly to mapper.readValue(bytes, clazz); 
Also, is there a way without registering the two variants of the same UDF?  Implementations of PluggableFunctionRegistry need to know call operatorTable#addInference and operatorTable#addDefault methods. 
Missing description. 
This really should be "When getting them" only, as it is not possible to set anything in a Row. 
`analysis.getFromDataSource(0)`?
can we put these lines in a method? They are used in multiple places. 
It is not primitive intentionally. To determine if it is not initialized. 
Could you add a description? What is this needed for? Thx! 
I'd use the charset from `AbstractAction` instead (server default charsets are rarely the expected ones, at least with the other one we have a sane default). 
nit: import order / spacing 
Great documentation. 
You could do `(Properties) Objects.requireNonNull(properties, "Properties cannot be null").clone();` and be done with it.
If `StatementExecutor` threw SQLException, you could catch it once in the execute method above. 
See my comment below about. I don't understand at a high level what buildURI() is supposed to do. This method seems to be overly complicated. I think it is OK to say that getConfigs() works only for configs coming from the same store/accessor. This will simplify the implementation significantly. You can just keep a reverse map from a ConfigKeyPath to a ConfigKeyURI that was used to generate it. Then populating the result is done using this map. 
Could you add some unit tests for this new logic? 
`Hclust` isn't very clear. I would prefer `HierarchicalClustering`
Eeek please remove before committing) 
Please add same comment.  ``` You should call this method after init your argumentsBundle.otherwise the transitionBundle will be not work. ```
I hope the -1 this may return is handled ( the `while` loop is huge, so I'm not sure how it is handled). 
Can you add 'final' keyword 
When blacklist is null, here needs check for null `patterns`. 
These header lines might be standard for apache license.  Does Apache publish the new lines?
docs for this and test. 
Formatting.  Will fix on merge.
Both these files are available in the central repo and are on the classpath of the plugin so we should not use reflection to access them..  
case in point: finalizeDatasetStateBeforeCommit() calls "datasetState.incrementJobFailures();" while this does not. 
is it possible to have `statuses.length == 0` ? in this case the call to `create()` below will fail 
Can this be handled in this method instead of recursing? 
Why do you override `equals` here? In most cases our "fake enum types" should be also checkable with reference equality - that means there is no need to override this method. 
@marcuslinke should i duplicate such test for netty? 
Please make sure the property is namespaced, say, with "gobblin.runtime." Thus, we can avoid clashes with user-defined properties. 
`OpenTSDBWriter2` should implement a reasonable `equals()` and `hashCode()` for OutputWriter deduplication (lombok is your friend). Make sure that the collaborators of this class also implement a reasonable `equals()` and `hashCode()`. A reasonable `toString()` would be a nice bonus. 
hmm we should probably change this to use something closer to a prepared statement. this looks like sql injection (not related to this issue obviously)
could you format this such that .execute() is obvious. 
Remove a trailing empty line for prettier code. 
This is no longer used. 
Remove space before `</li>` 
You shouldn't concatenate strings in Java like that, it can mess up translations (pretty sure lint will show a warning for it). Instead, you should use [string format parameters](https://stackoverflow.com/a/3656409), and I would make seperate TextViews for `Type` and type specific info.  So you would have `type = getString(R.string.type_simple)` for the first line, and `getString(R.string.type_simple_info, keepVersions)` for the second line.
Clients’ commands are appended to the Raft log in sequential order. But when clients’ requests are delivered out of order e.g. after leader changes, this can often lead to significantly increased communication between the leader and client. Queueing command’s to be written to the Raft log significantly decreases that communication, reduces latency, and thus reduces the frequency of timeouts for blocking commands.
The name is weird. Probably needs renaming.
Can you provide a more descriptive method name and short doc? 
Can remove this
nit: create a method to optionally submit events which checks if `eventSubmitter` is present. So you don't need to have this `if` blocks twice. 
add break? 
Since hashCode is generated off of the Class, couldn't this be a problem if you had two separate queries with the same Options but different option values?  (I.e. one query with TTL of 300, and another of 600) 
Instead of log-and-throw, rethrow a new exception type (defined as a static inner class in this file) whose string is the log message. This will result in better GAE logging. 
rename `symlink` to `someFileSymLink` to make it obvious what the link target is. 
Can you remove this todo while you're at it? 
given multiple if statements it will good to have braces around them to separate it out
Remove this
Before the endpoint group mark, we could also prepend `://`? 
`URI` might be worthwhile here, if we support `URL`. 
Please include `ee`. Or you can do `throw Throwables.propagate(ee);`. 
Add doc 
is true a good default? shouldn't it be false upon unknown types?
This line is not needed 
This doesn't compile, see https://travis-ci.org/SpongePowered/Sponge/builds/45614519#L514-L517 
What's the difference between 'offsetPath' and 'getLogFileOffsetPath'? 
You can actually use `CompositeClassLoader.class.isInstance(xStream.getClassLoader())` here.
`anyIdle()` randomness has some sense here: if for some reason we are assuming a peer to be best but it is invalid the sync may stuck since we will always be asking the same invalid peer. M.b. it makes sense to add some randomness to `getBestIdle()`?
Change to `Type` 
As discussed, we will shutdown the NioServer first. At that point we are sure no more reads will come in (neither will any writes go out). We then queue the poison and once the poison is encountered, just release the resources held by the rest of the RestRequestInfo instances in the queue. 
if you are changing this API, you need to change the `getBlob(String)` API also to `getBlob(String, GetBlobOptions)` 
let's keep `\n` here 
Why do you need this guy? `BraveServerInInterceptor` should do the job, right?
But does this pull request actually solve the heap errors? This line of code is still the weakest part since it takes array of int to get the pixels and only few code lines later it transforms it into ByteBuffer. So I guess the heap error will still be there in case the screenshot image is too big. Or the heap error you got was about something else? Do you still have the complete stacktrace?  Anyway, thanks for your pull request! It looks good. I am only going to perform some tests before accepting it. 
so, @leleuj , looking at other extractors/clients,  I guess this exception throwing is not the responsibility of `KerberosExtractor` and should be moved to `Kerberos(In)?DirectClient`.  there is one semantic nuance with SPNEGO/Kerberos - the browser (e.g. Firefox) do NOT send the credentials (`Authorization: Negotiate someTicket`) if server did not earlier provide a `WWW-Authenticate: Negotiate` header.   Thus a trully DirectClient don't quite make sense here. As it is now, the 401 indicates to repeat the request with credentials, but does do a redirect, so it's in between direct and indirect, I guess?  --- in my case, I'd like to combine session/cookie storage with Kerberos, so would not need renegotiate with every request. so in brief what I'd like to have sounds like indirect client.  would this happen automatically if it implemented an indirect client interface?  
Use OSGeo Header 
new line needed. 
why are you not using the mapper now?
@devrandom this is what I fixed. I just ensured the ! scope was correct. 
I think you need either volatile or an atomic reference here since moveToState can be executed by a thread other than the one running the fragment. 
Make these two static as well as final, and use UPPER_CAMEL.  Also use a better name that describes what they do.  Specifically what they are doing is that they cause an extra domain validation error if the incoming domain is equal to this, so how about calling them INVALID_LABEL_EXTRA_VALIDATION and INVALID_TLD_EXTRA_VALIDATION?  Or you could just inline them at all call sites; they're only used twice each and it's just a test.  Also, the value itself could be something clearer.  It's not immediately obvious why "extra.tld" would be a disallowed domain name in the extra logic.  If it were something like "disallowed.tld" that'd make the test more self-documenting.  (You'd also have to change the EPP XML files). 
I'm not sure what your policy is w/ Copyrights. I put my name since the file was new, but I have no problem changing it to jberkel. 
what was wrong with the previous code? the stream.read should be in a loop as Priyesh suggest.  
Unrelated change, revert! 
Remove, this case is not relevant for anyMatch
can you change the class passed to `getLogger()` from `OperatorToMetricsMapping` to `OperatorMetricRegistry` ? 
include these only if required.
Doesn't follow coding conventions, needs `{ ... }` 
I'm curious when this does and doesn't work.
two strategies at this moment - one without BASH because it's not available in Alpine linux for example, and another one with BASH 
`setTorchEnabled(boolean enabled)` 
toList() should return only JSON object. I understand that getObject(i) can return CBL Array() or CBL Dictionary() ?
`node.hasAttribute(PropertyDescriptorField.TYPE.attributeName())`?
``` mViewRawText = !mViewRawText; item.setChecked(mViewRawText); ```  ?
It is strongly recommended to add "<DOCTYPE..." at the top of your file, otherwise, TestNG may fail or not work as expected. 
null-check? 
I don't know whether this good thing or not... Probably good, if we know consider maps as containers.
You're not required to declare `Exception` because none of the lettuce client methods throws declared `Exception`s.  The point in the Clean Code book is that checked Exceptions break the Dependency Inversion Principle. Changes in the declared Exceptions require all callers to adopt the change even though callers might be not directly interested in the particular Exception. 
could you add documentation on all the constructors that require size to say something like "if unknown, make this -1"?
Ditto. You don't need to override this method. 
Should stay as `Boolean`. 
The condition on which this exception is thrown is the same everywhere (no value matching the identifier is found in the map).  Would it be possible to push the throwing of this exception in a single method deeper in the call stack? 
I don't think we need that if we drop the support for expression parameters as repository method parameters, right? 
This import is not used, right?
How about marking the policy as beta for this release? This would leave us some wiggle room if unexpected requirements come up. 
same as above 
Missing period. 
we would want to add a metric to track size of submap 
Please add Javadoc comments to all public classes and all non-overridden public/protected methods. 
Thanks for adding the Javadoc!
why the fully qualified package? You already imported java.util.\* (btw, might want to be more specific about that). 
the value of this would be consistent, right? It might be nice to show what it is (ex ip:port)  here's some assertj slickness..  ``` java     assertThat(span.binaryAnnotations)         .filteredOn(b -> b.key.equals(Constants.SERVER_ADDR))         .extracting(b -> new String(b.value))         .containsOnly("host:port"); ``` 
Please add flow control here, cf from stop is being ignored. 
May want some javadoc on what "routed" is.
I've missed the reasoning on this. Can you provide that @kashike . 
Is the sorting phase required by any `hiveSelectionPolicy`? 
Could pull the `List#size` call out of the loop 
It is not necessary to add comments for BDD. This generates noise and should be implicit.  
What is the reason for this limitation? 
That's correct and that's precisely what this test is for. 
I understand we may want to suppress things such as parse errors to stderr, but do we really want to prevent all output? If the app crashes, we would not show anything without these flags?
ditto
submitSuccessfulFilePublish is getting these from copyablefile in WUS 
Please use string substitution for debug calls so as to prevent the string concatenation from occurring in the release build.  ``` Timber.d("onCreate() :: received Intent with action = %s", getIntent().getAction()); ``` 
is it more conventional to call it "select"? 
For a different use-case we have something similar to this internally that allows one to specify that set of keys to either keep or drop and perform a rollup on the rest with an aggregate function such as sum. I don't think it is needed for this PR, but I wonder if having JmxMonitorRegistry support an aggregated value of some kind would be a better option for dealing with collisions. The other benefit is it would allow one to control the tree depth in jmx. 
Should we create a constant for the value 10 and use it here and when creating the insert string? 
`if (` instead of `if(` 
randomAccess is not thread safe.
In case of error, we probably want to throw an exception to ensure that this writer is disabled. Unless this can be a transient error. So probably in case of communication error, we have a good chance of recovery, otherwise probably not so much ...  Note: this is a minor point, I'll be happy to merge without it. 
Please use generated id. 
Don't inline this.
I don't know if the block should be a voxel, otherwise you wouldn't be able to know where the block is exactly. 
We should discuss this. I favor getting the request delegator through a static method if required and remove request delegator as an argument. 
Perhaps do a quick global search for getElementsBySeed and replace it with getElements.
Not needed. 
Why doesn't this include the `workflowId` in the query?
Would it be worthwhile to have a separate module with perf tests that could run independently? This test and others could live there and be un-commented. If we really wanted to, maybe we could have the junit suite call into those tests to verify that we didn't cripple performance with some change. 
This is MySQL specific.  The tests run with H2, unless there is a reason for them specifically not to. 
Please rename this function in something like `addSvgSignatureToGallery`, and rename also the other function generating the JPG/PNG image in `addJpgSignatureToGallery`. 
No need to check for null, it's just extra complexity. 
this is a repeat of the test already done. I think adding those entries is not really required. I would say these steps should be enough 1. `pause()` 2. Check paused variable and thread state 3. `reloadIndex()` 4. Check paused variable and thread state
might need a bit help to word this better. 
Automatic reminder. Nice.
I think this property should be mandatory, so no `if`/`else` clause.
Maybe getNextSiblingOfType will be more appropriate, but I am not sure.
this should throw, shouldn't it? 
Maybe we can just set the default properties all here and get rid of `defaultProducerConfig`? 
we don't use this. for methods. any reasons? 
Looks like this field could be made final 
Commented out lines should be removed 
ditto.
Is it possible some of the sessions are deserializable while some are not? It would seem nice to be able to return just the ones that are deserializable and delete the ones that are not. Otherwise, repeated attempts to find will always result in an empty result (until any configured timeout of the sessions that cannot be deserialized). I'm not sure how this can be accomplished in a way that supports any `FindByIndexNameSessionRepository` implementation, though. At least documenting the behavior in the JavaDoc for this method would be nice, I think. 
You need to grab the superordinate domain key off the host that you just loaded inside this transaction, in case anything else has written out a change to the host since you retrieved it.  It may be simplest to perform all actions inside a single transaction.
If you had an object `MetatstoreConfiguration` then this could just become: ``` final ObjectMapper mapper = new ObjectMapper(); mapper.writeValueAsString(metastoreConfig)
this is review comment 
No.
Maybe worth testing some mismatched types e.g. `Baz<R> extends Bar<String>` and some wildcard types, including `? super T` and the like? 
the same: use DartValueExpression.getExpressionList() and check first entry type 
Do we need this checked exception here? 
Name `build` or something like that would be better. 
extra `@return` 
@ahaines: I've refactored startup checks somewhat in this branch - I wonder if this might help with #154  
This line can be removed. 
This seems like it is leaking specific implementations. Can we come up with a way to move this around. Possibly GroupScan should have hasFiles() and getFiles()? 
For simplicity maybe take the users stuff and add it in this class (kind of like we do in the REST example) ? 
If we are going to worry about object allocation here, why not do something even simpler like using a `new SimpleEntry<>(ctx.getSqlObjectType(), name)` which should be much cheaper than assembling this string? 
consider creating a constant to define pool size. 
Sorry, still need to leave this in for the time being (pending some internal fixes currently in progress)
So...there's only one `Title` per game? 
I was not sure, since I'm already breaking the API, if I should rename this method right now or not. What do you think @adangel ?
pls remove 
Shouldn't this use the API rather than the fields directly?
Please use braces, even for single line statements.
Are you missing the isDirReadable() check here? 
Please note the semantic change here. 
Please fix formatting everywhere. There should be a space after "if" keyword. 
This code block is repeated over and over (lookup, register and log...) maybe make it a static method in Disposer? 
no more putIfAbsent ? 
You can just use `TimeUnit.SECONDS.toDays()`.
formatting. Are you sure you have run the formatter on these changed classes? 
Remove this line please.  
Ditto. 
Should be statically imported 
Tabs instead of spaces. Will fix on merge.
[ant:checkstyle] [WARN] /home/jasper/Documents/PokemonGo/PokeGOAPI-Java/library/src/main/java/com/pokegoapi/api/listener/SettingsListener.java:10: Block comment has incorrect indentation level 1, expected is 2, indentation should be the same level as line 17. [CommentsIndentation]
Could you please merge your changes with ft-119-testgroups branch. The thing is I have already merged you previous pull request into separate branch and in there I have removed this method together with the code that collects tags from layout reports. But the thing is that functionality of test groups is not yet completely finished.  
Could we merge `cassandraService` and `cassandraServiceDebug` into one service and test if MIME type parameter works as expected? 
any reason to not use `secondRecordFileSpan.getEndOffset()`?
@kashike   Why in the world does this take an Extent? The Schematic stores its blockstates.... 
jdbc_get_connection_count
extract a method?
No need for final here. 
Put white space after if and other keywords: http://htmlpreview.github.io/?https://raw.githubusercontent.com/ankidroid/Anki-Android/develop/docs/code_conventions/AnkiDroid_code_conventions.html#White Spaces Rule 37. 
You need not catch the exception. If the test throws an exception, the exception stack trace is printed by JUnit. 
If this was `holding(Matcher<? super V> matcher)` it might solve the generics problem you are seeing in EhcacheWithLoaderWriterBasicReplaceTest? 
there is no test for this currently?
What's the purpose of using `Long`? Why not just `long recordSize = toWrite.length`? 
These seem to be covered by `NUMBER_TYPES` already? 
I think possibly a new Retriever subclass is needed for all we may be able to simplify other things in a dedicated reliever over doing this check here, this also has a problem of tying in a specific operation to this functionality as opposed to operation calling specific retrievers in their handler.  
Missing final 
Probably wrong, this method should be called when player actually adds addon, not when deserializing it from NBT. 
I think it's better to blacklist than whitelist update() checks 
Any reason to add this instead of just using the constant? 
It's not, but I wonder if the alternatives wouldn't be worse? We could iterate over all mappings and remove them one by one but I'm slightly worried about the performance impact. 
Why not make this method return a `Map` instead of take in a `Map`. 
Consider moving to the Enum, also a bit confused on this - is it an "id" used for wicket communication - or is it human readable (in which case it should be translatable). 
Now that this can be run during catalog update, I'm not sure crashing Volt is good. Same as below. 
I'm not going to start renaming anything unless everyone agrees on what those names should be. 
A description of how this works and/or a CVE reference would be ideal here. Presumably this has been reported to JBoss already. 
A private field not accessed from anywhere else, but as a static it was retaining a state of changed across editings,  such that unchanged cards would return a status of changed. 
- [x] Ah, from this comment I see you did mean to call it `doBuildWithElixirc`, so you will want to fix [the name of that method](https://github.com/KronicDeth/intellij-elixir/pull/167/files#r34952046) 
Do we even need to be able to override this? (Or even the normal prompt, tbh.)
collectorRegistry?
Shouldn't this follow the pattern {} ? 
This can be local to the `getFeedPokemon` method.
![Codacy](https://www.codacy.com/assets/images/favicon.png) Issue found: [JUnit tests should include assert() or fail()](https://www.codacy.com/app/kronic-deth/intellij-elixir/pullRequest?prid=509027)
KeysController.class 
might want to throw an assertion to let you know when this is actually getting exercised 
Nice. I prefer your approach here. It's very clear what this does! 
You can use the single line Javadoc notation here. 
Any reason why you restrict to Exception here? If you want to provision for third-party mod error, don't you want to catch Throwable? 
? Accidental I guess 
![MAJOR](https://raw.githubusercontent.com/SonarCommunity/sonar-github/master/images/severity-major.png) Remove this unused private "bindProperty" method. [![rule](https://raw.githubusercontent.com/SonarCommunity/sonar-github/master/images/rule.png)](https://sonar.projects.vaadin.com/coding_rules#rule_key=squid%3AUnusedPrivateMethod) 
Do not use a raw type here 
Irrelevant documentation and it can be removed.
please use Boolean here 
Don't inline this
cool ! I already got prepared to that change in the TMS 
I have to validate eth addresses in a current project - I modified your version:  `String formattedAddress = address.replace("0x", "");  String lowercaseAddress = formattedAddress.toLowerCase();  String hash = Numeric.toHexStringNoPrefix(Hash.sha3(lowercaseAddress.getBytes()));      for (int i = 0; i < 40; i++) {          if (Character.isLetter(formattedAddress.charAt(i))) { `
Add proper spacing between methods and definitions.
integer -> short 
headers 
maybe subclassing would be better?
I agree getting all of them is kind of pointless. Clearing them has some potential to be useful though.  
I am not sure if this makes sense to be part of this PR or not. Does it even make sense to be included in AssertJ.  This one helped me a lot to figure out what exactly was the problem in the failing tests.
"http.status_code" if you don't mind.. that way it is the same value java vs otherwise 
copyright should be @stevespringett 
All the modifiers are redundant
I purely don't understand why this check is needed. Is any realistic example? 
**Serialization issue:** If you serialize/deserialize the image paths from a byte[] blob (using `serializer`), this needs to be of type `blob`, not `text`.
I am curious.. Did this test fail without the impersonation changes in AvroRecordReader? 
I don't think this is necessary. Builders are used only once, so why would a reset be needed? 
Can you rename this this and all the other classes in the package to follow standard naming conventions? This could just be called `Abs` it is already in the `udf.math` package 
Indentation is a bit odd here. 
Remove blank line. 
I think you need to cache these since each one will create a listener. This used to be done by cachingstoreprovider but I don't think that is in the Transient path anymore. 
The new line is because all the "related" error codes are grouped together. 
surely this needs a null check like has been done elsewhere?
version的判断没有用吧   
injected (sorry to correct your english!) 
Does this mean you will not update the topic schema to latest?
I'd put this condition first so it fails fast before even bothering checking the instance type 
Do not ignore but throw a new `EndpointGroupException`. 
A slight better pattern for this would be:  ``` java   try {     ...     fail("was supposed to throw");   } catch (Exception e) {     // This was expected.   } ``` 
Why OldApiHadoopFileInputSource and not HadoopFileInputSource? 
Can we use `try with resources` now since we migrated to Java 7. 
getBinds does check for null, others don't 
I guess you are the author xD
remember Apex is case insensitive
"subscribe"
Can you rename this to `JOB_RUN` to be consistent with the name? Ditto for other names. 
Ceki you are probably referring to this change (and several like it). It appends a char, not the string value of i + 1. It should be Integer.toString(i + 1) 
How about simplifying the whole method to these 2 lines  ```     final String flutterPath = PathUtil.getParentPath(PathUtil.getParentPath(PathUtil.getParentPath(dartSdk.getHomePath())));     return flutterPath.isEmpty() ? null : flutterPath; ```  ?  Or, even better, with Flutter-specific checks:  ```     final String dartPath = dartSdk.getHomePath();     final String suffix = "/bin/cache/dart-sdk";     return dartPath.endsWith(suffix) ? dartPath.substring(0, dartPath.length() - suffix.length()) : null; ```  ? 
My understanding is that a null resource pool indicates that the kind of resource you're interested in is not available.  A null ResourcePools on the other hand, may be something undesired to prevent null checks at the end of the call stack. 
This seems too long. 5ms or 10ms may be better. 
This does not seem to be used. Please remove.
are you supposed to return here? 
use `==` for enum equality check, please... 😃 
Please add javadocs 
SQLite does this automatically, maybe we don't really need it (you can just not use any of the `@Default[Type]` annotations on that field).
I'm not a big fan of special-casing this, but since it was already the case, and also the case for the single-column `in` operator, I left it as is. 
This looks redundant to me, since `testToCalendarWithDate()` already tests that the date has been set and `testToCalendarWithTimeZone()` already tests, that the timeZone has been set. 
HTTP Basic -> HTTP basic
You don't need treat `i == 0` specially here since you already set it to `new SingleRecordIterable<Object>(inputRecord).iterator()` above. You just need to start with `i = 1`; 
Done. 
You can use ````! Objects.equal(previousTarget, targetPos) ````
Can you add these type of TODO to your txt file. My next diff changes help in all this. 
same here, timeout? 
Ditto
This can be simplified by doing `if (instantiated.remove(instance)) && instance instance Closeable)` and thus dropping the remove after the block. 
We already have a private field `byName`in `ColumnDefintions`, why not simply expose it? 
Why store this as a final field if `callback.request()` is accessible from the enclosing `RequestHandler` instance? 
Just import it from the contract: `import com.ichi2.anki.FlashCardsContract.Note.DuplicateStrategy;` 
A description of how this works would be ideal here, especially if/how it interoperates with other ysoserial components. 
Would be cool to have a fast exit if `nbMatch == 0` as there was before 
Can you add more information to the javadocs to describe what parameters this method expects in the "state" parameter? e.g. KAFKA_BROKERS, USER_NEW_KAFKA_API, KAFKA_BROKERS, etc. 
nit: align with beginning of `(` above. 
It would be better to replace   `int rtlPosition = (mStepAdapter.getCount() - 1) - mCurrentStepPosition;`  with: `int currentStepPosition = mIsRtlEnabled ? (mStepAdapter.getCount() - 1) - mCurrentStepPosition : mCurrentStepPosition;`  and use that later in `return mStepAdapter.findStep(currentStepPosition);` to avoid code duplication 
Missing Javadoc 
@kuujo handled elsewhere now? 
Could you add a check that prevents a user from adding this service to more than one server? See `JettyService` for an example. 
use `Optional` here instead 
Missing space. Also, File.lastModified has only second precision under Java 7 on Linux so locking causes horrible race conditions. Fixing this is out of scope. 
Remove this method unless it is actually used in the test. 
why did you add this constructor? 
I realize this is out of scope for this review but it seems like this exception should be logged.
This assertions also fails with Eclipse:      Status in line 86: 		} catch (StubException e) { // $line-implicitExceptionTryCatch.catch$ expected:<[FUL]LY_COVERED> but was:<[PART]LY_COVERED>
we should avoid System.out.println() in production code
The `EmbeddedChannel`? Yeah that's fine - its not a real channel. The server closes that channel from its end though (thats what the asserts check). 
Hmm, really? I thought an authorizer returning true can finish the authorization process. Am I missing something?
indentation after catch 
Probably change the variable name to avoid two variables having the same name. How about change boolean variable to isCrossDCCallsEnabled? 
Is expr thread safe? with the move to support concurrency, is this a concern? 
Should throw javax.persistence.PersistenceException 
This code block seems to show up a number of times. Any chance we can refactor it into a method in one of the parent resource classes? 
Done 
The old code has a bug (returns number of notes in whole collection) because this is not supported in the old provider. Fixed in this PR. 
I guess these methods are not invoked anywhere else. So, would you consider removing the method altogether (3 methods) from NetworkMetrics. 
Could you please also rename `stateRoot(s)` to something like `nodeKey` or `nodeHash` to avoid confusion
There is quite a bit of plumbing here to get right -- for each new schema added. Please factor out the common code into a (private) implementation method. Else, we'll be chasing bugs forever as people modify one copy but miss the others.
Missing generic on return
merge ifs? 
use `this(dockerImageName, "test", "test", "test")` 
Shouldn't here throw `EOFException` instead of `MessageFormatException`? Because `readPayload`, `unpackString` and `getNextFormat` throws `EOFException`. 
we need a gauge metric here to ensure the thread is running