We create an injectable LockHandler that just calls the static
Lock.executeWithLocks function.
I'm not sure what's the correct place to put the LockHandler. I think
model/server is only appropriate for the actual datastore lock. This is a "per request" lock, so maybe request/lock?
-----------------------------
This is the initial step in adding the "lock implicitly released on request death" feature, but it's also useful on its own - easier to test Actions when we can use a fake lock.
To keep this CL simple, we keep using the old Lock as is in most places. We just choose a single example to convert to LockHandler to showcase it. Converting all other uses will be in a subsequent CL.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=167357564
It was buggy (didn't work) and was never actually used.
Why never actually used: for it to be used executeWithLock has to be called
with different requesters on the same lockId. That never happend in the code.
How it was buggy: Logically, the queue is deleted on release of the lock (meaning it was
meaningless the only time it mattered - when the lock isn't taken). In
addition, a different bug meant that having items in the queue prevented the
lock from being released forcing all other tasks to have to wait for lock
timeout even if the task that acquired the lock is long done.
Alternative: fix the queue. This would mean we don't want to delete the lock on release (since we want to keep the queue). Instead, we resave the same lock with expiration date being START_OF_TIME. In addition - we need to fix the .equals used to determine if the lock the same as the acquired lock - instead use some isSame function that ignores the queue.
Note: the queue is dangerous! An item (calling class / action) in the first place of a queue means no other calling class can get that lock. Everything is waiting for the first calling class to be re-run - but that might take a long time (depending on that action's rerun policy) and even might never happen (if for some reason that action decided it was no longer needed without acquiring the lock) - causing all other actions to stall forever!
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=163705463
This makes the code more understandable from callsites, and also forces
users of this function to deal with the situation where the registrar
with a given client ID might not be present (it was previously silently
NPEing from some of the callsites).
This also adds a test helper method loadRegistrar(clientId) that retains
the old functionality for terseness in tests. It also fixes some instances
of using the load method with the wrong cachedness -- some uses in high-
traffic situations (WHOIS) that should have caching, but also low-traffic
reporting that don't benefit from caching so might as well always be
current.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=162990468
This standardizes use of annotations/inheritance/formatting across
tests, to make the code more legible and consistent.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=161810734
The billing account map will be serialized in the following format:
{currency1=id1, currency2=id2, ...}
In order for the output to be deterministic, the billing account map is stored as a sorted map.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=161075814
The absence of these fields causes RDE failures, so they are in effect
required on any functioning registry system. We are currently
experiencing problems in sandbox caused by null values on these fields.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=155474895
Also added corresponding getters and setters for the new field. Note that
nothing has changed on the RDAP front for now, as the CL&D only concerns WHOIS.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=155116134
hasMessageThat() was added in Truth 0.32 and we are already on that
version in the Nomulus release.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=153095111
This is a follow-up to Lai's refactoring of the get reservation types
code to return a set rather than a single type. Since we're always
returning a set now, the more natural way to represent a label that is
not reserved is to return an empty set rather than a set containing
UNRESERVED.
Also fixes some minor style issues I ran across regarding static
importing and test method naming that I ran across (no logic
implications).
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=151132116
This is the first step in the migration to remove the need to load all of
the premium list entries every time the cache expires (which causes slow-
downs). Once this is deployed, we can re-save all premium lists, creating
the bloom filters, and then the next step will be to read from them to
more efficiently determine if a label might be premium.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=147525017
There are still some options in RegistryConfig that can't be configured
in YAML, but it's not clear why anyone would need to change them from
their default values.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=146482435
This is the final preparatory step necessary in order to load and load
configuration from YAML in a static context and then provide it either via
Dagger (using ConfigModule) or through RegistryConfig's existing static
functions.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=143819983
We are now ready to begin configuration using YAML, mediated by ConfigModule.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=143818507
The next step will be to get rid of RegistryConfig descendants and RegistryConfigLoader entirely.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=143812815
This primarily addresses issues with TMCH testing mode and email sending utils.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=143710550
In my previous refactoring I failed to realize that getResource() was not
returning the contents of the file.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=142065550
I also moved to a non-concurrent modification syncing model. It was adding more
complexity than was justified just to have two requests going simultaneously
instead of one. The API doesn't reliably allow much more than that anyway.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=141210192
This defaults to null, and leaving it to null now simply disables reserved terms
exporting, rather than throwing an error every time the action runs.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=138763161
This doesn't change the end result of a successful run, though this is what a typical flow looks like prior to this fix:
Consider a sheet with 10 data rows (+ 1 header row = 11). A 10-row data set will call worksheet.setRowCount(10), which truncates the last row of the existing sheet. This row will eventually be added again in the last for loop, but if the synchronizer fails mid-sync, this last row will remain dropped. This fix will prevent this last row from being dropped.
This doesn't fix the broader issue of SheetSynchronizer not behaving transactionally -- that's a different can of worms.
See the linked bug for an instance where the synchronizer failed mid-run and dropped a data row as a result.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=136398109
This is to better distinguish between an LRP "token" (the string passed along in EPP) and the datastore entity that contains the token and all metadata.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=135943480
aka regexing for fun and profit.
This also makes sure that there are no statements after the
throwing statement, since these would be dead code. There
were a surprising number of places with assertions after
the throw, and none of these are actually triggered in tests
ever. When I found these, I replaced them with try/catch/rethrow
which makes the assertions actually happen:
before:
// This is the ExceptionRule that checks EppException marshaling
thrown.expect(FooException.class);
doThrowingThing();
assertSomething(); // Dead code!
after:
try {
doThrowingThing();
assertWithMessage("...").fail();
} catch (FooException e) {
assertSomething();
// For EppExceptions:
assertAboutEppExceptins().that(e).marshalsToXml();
}
To make this work, I added EppExceptionSubject.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=135793407
This also changes the default number of mapper shards in tests to 2, which is
the number of EppResourceIndex buckets in unit tests. Running more shards than
there are buckets causes unnecessary test load.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=135520601
It's best to be consistent and use the same thing everywhere. "clientId" was
already used in more places and is shorter and no more ambiguous, so it's the
logical one to win out.
Note that this CL is almost solely a big Eclipse-assisted refactoring. There are
two places that I did not change clientIdentifier -- the actual entity field on
Registrar (though I did change all getters and setters), and the name of a
column on the exported registrar spreadsheet. Both would require data
migrations.
Also fixes a few minor nits discovered in touched files, including an incorrect
test in OfyFilterTest.java and some superfluous uses of String.format() when
calling checkArgument().
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=133956465
This change replaces all Ref objects in the code with Key objects. These are
stored in datastore as the same object (raw datastore keys), so this is not
a model change.
Our best practices doc says to use Keys not Refs because:
* The .get() method obscures what's actually going on
- Much harder to visually audit the code for datastore loads
- Hard to distinguish Ref<T> get()'s from Optional get()'s and Supplier get()'s
* Implicit ofy().load() offers much less control
- Antipattern for ultimate goal of making Ofy injectable
- Can't control cache use or batch loading without making ofy() explicit anyway
* Serialization behavior is surprising and could be quite dangerous/incorrect
- Can lead to serialization errors. If it actually worked "as intended",
it would lead to a Ref<> on a serialized object being replaced upon
deserialization with a stale copy of the old value, which could potentially
break all kinds of transactional expectations
* Having both Ref<T> and Key<T> introduces extra boilerplate everywhere
- E.g. helper methods all need to have Ref and Key overloads, or you need to
call .key() to get the Key<T> for every Ref<T> you want to pass in
- Creating a Ref<T> is more cumbersome, since it doesn't have all the create()
overloads that Key<T> has, only create(Key<T>) and create(Entity) - no way to
create directly from kind+ID/name, raw Key, websafe key string, etc.
(Note that Refs are treated specially by Objectify's @Load method and Keys are not;
we don't use that feature, but it is the one advantage Refs have over Keys.)
The direct impetus for this change is that I am trying to audit our use of memcache,
and the implicit .get() calls to datastore were making that very hard.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=131965491
