fesl / figgo

Sistema de gestão núcleos culturais
http://www.figgo.com.br
GNU General Public License v3.0
4 stars 0 forks source link

Extrato não está mostrando exatamente as ultimas transações #91

Closed daniloqueiroz closed 12 years ago

vitoravelino commented 12 years ago

@tigreped Já perceberam isso ocorrendo no Mundo hoje?

@DaniloQueiroz Recuperar por timestamp desc resolve ou o buraco é mais embaixo?

daniloqueiroz commented 12 years ago

Acabei de dar uma olhada em https://github.com/fesl/figgo/blob/master/src/main/java/br/octahedron/figgo/modules/bank/data/BankTransactionDAO.java e parece que o buraco é mais embaixo, só não faço ideia onde...

tigreped commented 12 years ago

Tipo, não vi o código, mas esse select aí, vocês tem certeza que tá buscando o intervalo de datas do registro, ou a data na qual o registro em questão foi feito no banco?

Pq tipo, os dados foram inicialmente importados em fevereiro, mais ou menos. Então, se tiver buscando pela data de inserção do registro no banco, aí vai estar errada a lógica.

Teria que buscando os registros observando se o intervalo passado como parametro de busca contempla o valor da data de registro de cada registro, né. A impressão que eu tenho pelo uso da busca de extrato - que eu estou supondo que esteja com implementação semelhante - está se comportando dessa forma.

daniloqueiroz commented 12 years ago

Os registros (BankTransactions) só tem uma campo com data, chamado timestamp. Quando se efetua uma operação utilizando o sistema este timestamp é preenchido pelo sistema, indicando a hora que a transação ocorreu. Não temos uma data de registro no sistema e uma data da operação. Na ocasião da importação dos dados, a inserção foi feita de forma direta ao banco, então esta lógica foi transpassada, e foi mantido como timestamp as datas que estavam nas planilhas.

Pra mim das duas umas, ou tem algum erro sutil no algoritmo da busca, ou o é algo mais sinistro, relacionado ao DS do google, consistência de dados e coisas que o valham.

tigreped commented 12 years ago

Entendi. Oxalá seja um erro no algoritmo de busca, então;

tigreped commented 12 years ago

Esse BankTransactionDao (e os outros DAO em geral) foram gerados automaticamente, correto?

tigreped commented 12 years ago

Problema era o comparator que estava ignorando um problema de casting entre long e int. O problema é que alterar isso implica numa mudança na maneira de calcular o saldo, já que há uma reorganização das transações por data e esta mudança precisa de uma avaliação mais aprofundada.

daniloqueiroz commented 12 years ago

Entendi o problema é até tenho um patch pra ele, só não entendi porque Isso muda a forma de calcular o saldo.

daniloqueiroz commented 12 years ago
    private class BankTransactionComparator implements Comparator<BankTransaction> {
        public int compare(BankTransaction o1, BankTransaction o2) {
            int comp = o1.getTimestamp().compareTo(o2.getTimestamp());
            return (comp != 0) ? comp : o1.getId().compareTo(o2.getId()); 
        }
    }

Isso deve corrigir o comparador. De toda forma, isso não deve(ria) afetar o calculo do saldo, pois este comparador é usado após recuperar as transações usadas pelo saldo do banco, ele apenas ordena as que foram recuperadas, mas não adiciona/exclui nenhuma transação.

vitoravelino commented 12 years ago

Entendido e corrigido aqui, mas lembra o saldo não guarda o timestamp da última transação recuperada? Com essa mudança a ordem muda (pra correta) e pode causar alguma inconsistência no cálculo do valor. Quando colocar isso em produção o ideal é zerar o lastTimestamp de todas as contas do banco para garantir que tudo está sendo calculado corretamente.

daniloqueiroz commented 12 years ago

Na verdade, acho que não. Esse comparador é usado pra fazer merge das transações cuja conta é destino e as que a conta é origem, certo? Mas quando se calcula o saldo, é pedido o Banco de dados todas as transações da conta a partir do último timestamp, então é feito o merge. Mesmo que as transações de origem e destino estejam desordenadas todas elas são iguais ou maior que o último timestamp. O algoritmo de cálculo de saldo não se baseia na premissa de que as transações estão ordenadas. Agora claro, isso é a teoria, talvez a implementação esteja falha, é preciso revisa la. Até onde eu lembro ela tem testes, então se puder dar uma revisada, se for o caso adicionar mais alguns testes seria ótimo. Vou tentar colocar no wiki como funciona.

vitoravelino commented 12 years ago

Por mais que viesse desordenado, o fato de pegar as transações a partir de um timestamp e guardar o último maior timestamp deveria garantir a consistência. Eu até peguei o banco local de @tigreped pra testar aqui porque ele tem um cenário parecido com o de produção, mas o gae sdk não aceitou. O estranho é que o saldo está errado no cenário dele (confere?).

Sim, tem testes e estão passando. Havia dado problema no teste usando mock, mas usando o compare() da forma como você mencionou resolveu.

Fiquei confuso porque o updateLastTransactionData() (BankAccount) parece não guardar todos os transactionId. Foi proposital? Se sim, não funcionaria guardar apenas em um Long ao invés de um Collection<Long> já que teoricamente não teremos transações duplicadas com o mesmo id (que é gerado pelo datastore - se ocorrer foderia porque não seria duplicado e trataríamos como se fosse)? Não entendi essa decisão de usar a coleção.

daniloqueiroz commented 12 years ago

A ideia do updateLastTransactionData e deste TransactionIds é guardar todos os transactions ID utilizados para o saldo cujo o timestamp é igual ao maior timestamp.

Talvez no final das contas esta nem seja uma preocupação necessária... minha questão foi só se ocorrer duas transações no mesmo timestamp e logo em seguida pedir o saldo, se eu pedir maior que o time stamp posso perder dados, se pedir igual ao timestamp posso correr o risco de computar duas vezes a mesma transação.

Fiz isso pensando em alguma eventual inconsistência de escrita / leitura tipo:

vitoravelino commented 12 years ago

Entendido. Em um mundo onde os ids fossem sempre crescentes isso não seria necessário. Mais um viva ao datastore! :P

Agora é tentar entender o porque de ter calculado o saldo errado se aparentemente o algoritmo garante consistência. Mas ainda tinha um caso de não aparecer a última transação no teste que fizemos ontem. Muito bizarro. Queria conseguir reproduzir isso aqui no meu ambiente, mas está difícil. :/

Vou fazer push dos commits aqui.

daniloqueiroz commented 12 years ago

Como eu disse o algoritmo, em teoria, garante consistência :p

tigreped commented 12 years ago

Pois é, aqui no ambiente local - e verificamos que no de produção também - alguns somatórios de saldos não estão corretos, como o cálculo do lastro e o saldo individual de um usuário.